Re: [PATCH] Assert on invalid bitmap iterations

[Richard Biener]

On Fri, 7 Oct 2016, Bernd Schmidt wrote:

> On 10/07/2016 09:20 AM, Richard Biener wrote:
> > Index: gcc/sched-deps.c
> > ===================================================================
> > --- gcc/sched-deps.c        (revision 240829)
> > +++ gcc/sched-deps.c        (working copy)
> > @@ -3992,8 +3992,14 @@ remove_from_deps (struct deps_desc *deps
> >    removed = remove_from_dependence_list (insn,
> > &deps->last_pending_memory_flush);
> >    deps->pending_flush_length -= removed;
> > 
> > +  unsigned to_clear = -1U;
> >    EXECUTE_IF_SET_IN_REG_SET (&deps->reg_last_in_use, 0, i, rsi)
> >      {
> > +      if (to_clear != -1U)
> > +   {
> > +     CLEAR_REGNO_REG_SET (&deps->reg_last_in_use, to_clear);
> > +     to_clear = -1U;
> > +   }
> >        struct deps_reg *reg_last = &deps->reg_last[i];
> >        if (reg_last->uses)
> >     remove_from_dependence_list (insn, &reg_last->uses);
> > @@ -4005,8 +4011,10 @@ remove_from_deps (struct deps_desc *deps
> >     remove_from_dependence_list (insn, &reg_last->clobbers);
> >        if (!reg_last->uses && !reg_last->sets && !reg_last->implicit_sets
> >       && !reg_last->clobbers)
> > -        CLEAR_REGNO_REG_SET (&deps->reg_last_in_use, i);
> > +   to_clear = i;
> >      }
> > +  if (to_clear != -1U)
> > +    CLEAR_REGNO_REG_SET (&deps->reg_last_in_use, to_clear);
> 
> That looks pretty bad. What's the issue, we can't clear the current bit while
> iterating over a bitmap? Is that fixable - it seems like something people are
> likely to want to do?

I think it would be possible if we'd had a bitmap_clear_bit variant
that uses the iterator because it needs to update the iterator which
keeps a pointer to the current bitmap_element plus a cached value
of the current bitmap_word.  Might be harder for the EXECUTE_IF_OP_
variants.

Currently if you remove the current (last bit) in a bitmap_element
it will get unlinked and put onto the freelist by bitmap_clear_bit
which then causes the iteration to immediately terminate (because
that bitmap element ->next pointer is now NULL).

> Here, if necessary I'd prefer we create a to_clear bitmap and perform an
> and_compl operation after the loop.

But that's way more expensive -- you allocate memory and perform an
additional loop over the bitmap.

I think the main issue is that it is not documented what is safe to do
(and what are the results) when you modify a bitmap while you are
iterating over it.

The bitmap iterator is optimized to optimize into a nice loop for
the bits in one bitmap word at least.

Richard.