Re: RFA: Rework FOR_BB_INSNS iterators

[Steven Bosscher <stevenb dot gcc at gmail dot com>]

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.


> 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.


> 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...



> +/* 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.

I would not approve this patch, but let's wait what others think of it.

Ciao!
Steven