[PATCH] Fix comments that refer to ENTRY_{BLOCK|EXIT}_PTR

[David Malcolm]

On Wed, 2013-11-20 at 11:28 -0700, Jeff Law wrote:
> On 11/20/13 10:03, David Malcolm wrote:
[...]
> > There are three places the patch doesn't touch:
> >
> > (A) cfgbuild.c (make_edges) has this comment:
> >    /* By nature of the way these get numbered, ENTRY_BLOCK_PTR->next_bb block
> >       is always the entry.  */
> > where the meaning wasn't immediately clear to me - what is the second
> > "entry" here?  (I haven't looked in detail at the algorithm).
> Hmm, I think "the entry" should be "the exit".  My recollection is 
> ENTRY_BLOCK is block #0 and EXIT_BLOCK is block #1. 
Yes

> But that would mean 
> we're making a edge from the entry to the exit?!?

But the entry block's "next_bb" isn't the exit block: for example, in
the following it's id 4, rather than 1:

(gdb) p cfun->cfg->x_entry_block_ptr 
$8 = <basic_block 0x7ffff042a0d0 (ENTRY)>
(gdb) p cfun->cfg->x_entry_block_ptr->next_bb
$9 = <basic_block 0x7ffff042aa28 (4)>

I believe that the initial version of that comment first appeared in
r25450 in flow.c:make_edges (in 1999) by rth "Flow rewrite to use basic
block structures and edge lists.":

 /* By nature of the way these get numbered, block 0 is always the
entry.  */
 make_edge (ENTRY_BLOCK_PTR, BASIC_BLOCK (0), EDGE_FALLTHRU);

r45504 (65f34de51669d0fe37752d46811f848402c274e4) moved make_edges (and
thus the comment) from flow.c to cfbuild.c

r53522 removed the comment (4c5da23833f4604c04fb829abf1a39ab6976e7b2:
"Basic block renumbering removal.") aka:
http://gcc.gnu.org/ml/gcc-patches/2002-05/msg01207.html

r53537 (b3d6de8978fd2208885e98b19a91c9d29c170af5) reverted that change,
reinstating the comment.

r53695 (345ac34a19ee8fefc1d767f4eb9103a781c641d3) updated make_edges to
work with basic_block ptrs rather than indices into the BASIC_BLOCK()
array.

If I'm reading r53804 correctly (in git it's
4c26117afc55fbf0b998d0bf25f1ab56da4dd180), way back in 2002, basic
blocks were accessed by index into BASIC_BLOCK() with limit
n_basic_blocks, and the entry and exit blocks appear to have been
*outside* of this array.  r53804 updated things to use FOR_EACH_BB, thus
visiting all basic blocks including entry and exit.   It appears that at
some point the entry and exit blocks have become part of the
x_basic_block_info vec; it was this change that updated:
-  /* By nature of the way these get numbered, block 0 is always the
entry.  */
+  /* By nature of the way these get numbered, ENTRY_BLOCK_PTR->next_bb
block
+     is always the entry.  */
   if (min == ENTRY_BLOCK_PTR->next_bb)
     cached_make_edge (edge_cache, ENTRY_BLOCK_PTR, min,
                      EDGE_FALLTHRU);

It's late and I'm not familiar enough the history of and current state
of the BB splitting code to figure out what this comment is saying (and
whether it's true).   My current guess (and it's a guess) is that it
relates to the 2002 removal of renumbering of basic blocks.

> > (B) graphite-scop-detection.c (scopdet_basic_block_info) has:
> >    /* XXX: ENTRY_BLOCK_PTR could be optimized in later steps.  */
> > where the meaning isn't clear to me - whether this is a note about a
> > possible further optimization, or a warning that the entry could be
> > changed.
> Me neither.  I try to avoid the graphite bits.
> 
> >
> > (C) tree-cfg.c (move_sese_region_to_fn): line 6899 has:
> >       FIXME, this is silly.  The CFG ought to become a parameter to
> >       these helpers.  */
> > where cfun is perhaps being unecessarily manipulated, and perhaps we
> > could actually gain a speedup from the macro removal work; if so, this
> > feels like a followup patch.
> That could be a follow-up.  Not sure if it really buys us much since 
> we're going to have to extract the CFG from the target cfun anyway.  I 
> guess we avoid pushing/popping them.
> 
> The downside is you'll have to pass the CFG into all those make_edge 
> calls, which 99.9% of the time is pointless.

I started looking at this, thinking to turn them into member functions
of the control_flow_graph type, but it rapidly hits other global state:
the cfghooks, and evicting the df cache.  So I stopped :)

> As for the patch itself, it's fine.

Thanks.  Committed to trunk as r205182.