Re: RFA: Rework FOR_BB_INSNS iterators

[Richard Sandiford <rdsandiford@googlemail.com>]

Steven Bosscher <stevenb.gcc@gmail.com> writes:
> On Sat, Jun 7, 2014 at 7:54 PM, Richard Sandiford wrote:
>> The two parts of the loop condition are really handling two different
>> kinds of block: ones like entry and exit that are completely empty
>> and normal ones that have at least a block note.  There's no real
>> need to check for null INSNs in normal blocks.
>
> Block notes should go away some day, they're just remains of a time
> when there was no actual CFG in the compiler.

Yeah.  I suppose when that happens empty blocks would look just like
entry and exit as far as these iterators go.

>> Also, refetching NEXT_INSN (BB_END (BB)) for each iteration can be expensive.
>> If we're prepared to say that the loop body can't insert instructions
>> for another block immediately after BB_END,
>
> This can happen with "block_label()" if e.g. a new jump is inserted
> for one reason or another. Not very likely for passes working in
> cfglayout mode, but post-RA there may be places that need this
> (splitters, peepholes, machine dependent reorgs, etc.).
>
> So even if we're prepared to say what you suggest, I don't think you
> can easily enforce it.

Probably true.  But if we want to allow insertions after BB_END,
we need to make FOR_BB_INSNS_SAFE handle that for INSN == BB_END too.

The alternative definition:

  for (rtx INSN = BB_HEAD (BB), INSN##_cond_ = INSN, INSN##_next_;	\
       INSN##_cond_ 							\
	 && (INSN##_next_ = NEXT_INSN (INSN),				\
	     INSN##_cond_ = BB_END (BB),				\
	     true);							\
       INSN##_cond_ = (INSN == INSN##_cond_ ? NULL_RTX : (rtx) 1),	\
       INSN = INSN##_next_)

works too.  It isn't quite as fast, but obviously correctness comes first.

>> It's easier to change these macros if they define the INSN variables
>> themselves.
>
> If you're going to hack these iterators anyway (much appreciated BTW),
> I suggest to make them similar to the gsi, loop, edge, and bitmap
> iterators: A new "insn_iterator" structure to hold the variables and
> static inline functions wrapped in the macros. This will also be
> helpful if (when) we ever manage to make the type for an insn a
> non-rtx...

Well, with this and...

>> +/* For iterating over insns in a basic block.  The iterator allows the loop
>> +   body to delete INSN.  It also ignores any instructions that the body
>> +   inserts between INSN and the following instruction.  */
>> +#define FOR_BB_INSNS(BB, INSN)                                         \
>> +  for (rtx INSN = BB_HEAD (BB), INSN##_cond_ = INSN, INSN##_next_,     \
>> + INSN##_end_ = INSN ? NEXT_INSN (BB_END (BB)) : NULL_RTX; \
>> + INSN##_cond_ && (INSN##_next_ = NEXT_INSN (INSN), true); \
>> +       INSN = INSN##_next_,                                            \
>> +       INSN##_cond_ = (INSN != INSN##_end_ ? (rtx) 1 : NULL_RTX))
>
> This just makes my eyes hurt...
>
>
> What about cases where a FOR_BB_INSNS is terminated before reaching
> the end of a basic block, and you need to know at what insn you
> stopped? Up to now, you could do:
>
>   rtx insn; basic_block bb;
>   FOR_BB_INSNS (bb, insn)
>     {
>       ... // do stuff
>       if (something) break;
>     }
>   do_something_with (insn);
>
> Looks like this is no longer possible with the implementation of
> FOR_BB_INSNS of your patch.

...this I suppose it depends where we want to go with these iterators.
I'm hoping eventually we'll move to C++11, where the natural way of
writing the loop would be:

  for (rtx insn : bb->insns ())

(Or "auto *" instead of "rtx" if you prefer.)

And I think the idiom of having the FOR_* macro define the iterator
variable is much closer to that than:

  rtx insn;
  FOR_BB_INSNS (iter, bb, insn)

would be.

It's true that you can't leave "insn" with a signficant value after
the loop, but no current code wants to do that.  Personally I like
the fact that loops that do want to set a final value have to make
that explicit.  When the vast majority (currently all) instances of:

  FOR_BB_INSNS (bb, insn)

treat "insn" as local to the loop, it's helpful when the exceptions
are obvious.

I think if anything the patch would make it easier to change the
type of insns to something other than rtx.  It would just mean changing
the "rtx" at the start of the two "for" loops to the new type,
whereas at the moment you would need to change all the separate
"rtx insn"s.  (In particular, it's common for "insn" to be defined
on the same line as other rtx variables that might need to stay
as "rtx"es after the change.)

Thanks,
Richard