| Message ID | 1384967011.11568.154.camel@surprise |
|---|---|
| State | New |
| Headers | show |
On 11/20/13 10:03, David Malcolm wrote: > > I went through the comment lines, rewording the ones where the meaning > was obvious to me. Attached is a patch that does so; successfully > compiled stage1; OK for trunk? (these are just changes to comments, so > not sure a full bootstrap is necessary). Yea, if you're just changing a comment, just compiling enough to ensure you didn't make a goof like forgetting to close the comment is fine. > > 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. But that would mean we're making a edge from the entry to the exit?!? > > (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. As for the patch itself, it's fine. jeff
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.
Hi, On Wed, 20 Nov 2013, Jeff Law 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". No, "the entry" is correct but misleading. There are two entry categories: ENTRY_BLOCK (containing no instructions, and once was block -1) and the first entered basic block containing instructions, which by convention is ENTRY_BLOCK_PTR->next_bb (and once was numbered 0), sometimes called "the entry". The reason for this seemingly useless additional BB is the potential support for functions with multiple entry points. All these real entry points would be successors of the ENTRY_BLOCK, simply because normal CFG algorithms are usually expressed with a single entry point of the CFG. The support for multiple entry points was never completed, so there's only one real entry block, which nevertheless is the (single) successor of ENTRY_BLOCK. Same for EXIT_BLOCK, all blocks containing real code that make the function exit ("the exits") are predecessors of the single EXIT_BLOCK, so that the reverse CFG also has only a single entry point. > My recollection is ENTRY_BLOCK is block #0 and EXIT_BLOCK is block #1. > But that would mean we're making a edge from the entry to the exit?!? Ciao, Michael.
commit f9ac8591f2ef893f489a0cec812c3198d8eaea5c
Author: David Malcolm <dmalcolm@redhat.com>
Date: Mon Nov 18 21:16:04 2013 -0500
Reword comments that mention ENTRY_BLOCK_PTR and EXIT_BLOCK_PTR macros
gcc/
* cfg.c (dump_edge_info): Remove redundant comment.
* cfgcleanup.c (outgoing_edges_match): Reword reference to
EXIT_BLOCK_PTR in comment.
(try_optimize_cfg): Likewise.
* cfgrtl.c (last_bb_in_partition): Likewise.
* cgraph.c (cgraph_node_cannot_return): Likewise.
* function.c (thread_prologue_and_epilogue_insns): Likewise.
* graphite-scop-detection.c (scopdet_basic_block_info): Likewise.
* ipa-split.c (consider_split): Likewise.
* profile.c (find_spanning_tree): Likewise.
* sched-int.h (common_sched_info_def.add_block): Likewise.
* dominance.c (calc_dfs_tree_nonrec): Reword references in
comments to now removed ENTRY_BLOCK_PTR and EXIT_BLOCK_PTR macros.
* tree-cfgcleanup.c (cleanup_control_flow_bb): Reword references
in comments to now removed ENTRY_BLOCK_PTR macro.
(tree_forwarder_block_p): Reword reference in comment to
EXIT_BLOCK_PTR.
* tree-inline.c (copy_cfg_body): Reword references in comments to
now removed ENTRY_BLOCK_PTR macro.
* tree-ssa-propagate.c (ssa_prop_init): Likewise.
* tree-scalar-evolution.h ( block_before_loop): Likewise. Add
a comma to the comment to clarify the meaning.
diff --git a/gcc/cfg.c b/gcc/cfg.c
index e35eee9..6bceca5 100644
--- a/gcc/cfg.c
+++ b/gcc/cfg.c
@@ -473,8 +473,6 @@ dump_edge_info (FILE *file, edge e, int flags, int do_succ)
&& (flags & TDF_SLIM) == 0)
do_details = true;
- /* ENTRY_BLOCK_PTR/EXIT_BLOCK_PTR depend on cfun.
- Compare against ENTRY_BLOCK/EXIT_BLOCK to avoid that dependency. */
if (side->index == ENTRY_BLOCK)
fputs (" ENTRY", file);
else if (side->index == EXIT_BLOCK)
diff --git a/gcc/cfgcleanup.c b/gcc/cfgcleanup.c
index 9c12610..dbaee96 100644
--- a/gcc/cfgcleanup.c
+++ b/gcc/cfgcleanup.c
@@ -1535,7 +1535,7 @@ outgoing_edges_match (int mode, basic_block bb1, basic_block bb2)
edge e1, e2;
edge_iterator ei;
- /* If we performed shrink-wrapping, edges to the EXIT_BLOCK_PTR can
+ /* If we performed shrink-wrapping, edges to the exit block can
only be distinguished for JUMP_INSNs. The two paths may differ in
whether they went through the prologue. Sibcalls are fine, we know
that we either didn't need or inserted an epilogue before them. */
@@ -2684,7 +2684,7 @@ try_optimize_cfg (int mode)
}
delete_basic_block (b);
changed = true;
- /* Avoid trying to remove ENTRY_BLOCK_PTR. */
+ /* Avoid trying to remove the exit block. */
b = (c == ENTRY_BLOCK_PTR_FOR_FN (cfun) ? c->next_bb : c);
continue;
}
diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c
index 7ad3872..63f44af 100644
--- a/gcc/cfgrtl.c
+++ b/gcc/cfgrtl.c
@@ -1795,7 +1795,7 @@ last_bb_in_partition (basic_block start_bb)
if (BB_PARTITION (start_bb) != BB_PARTITION (bb->next_bb))
return bb;
}
- /* Return bb before EXIT_BLOCK_PTR. */
+ /* Return bb before the exit block. */
return bb->prev_bb;
}
diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 624d492..009a165 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -2334,7 +2334,7 @@ cgraph_node_cannot_return (struct cgraph_node *node)
and thus it is safe to ignore its side effects for IPA analysis
when computing side effects of the caller.
FIXME: We could actually mark all edges that have no reaching
- patch to EXIT_BLOCK_PTR or throw to get better results. */
+ patch to the exit block or throw to get better results. */
bool
cgraph_edge_cannot_lead_to_return (struct cgraph_edge *e)
{
diff --git a/gcc/dominance.c b/gcc/dominance.c
index 3d88c0d..5ece3f6 100644
--- a/gcc/dominance.c
+++ b/gcc/dominance.c
@@ -227,7 +227,7 @@ calc_dfs_tree_nonrec (struct dom_info *di, basic_block bb, bool reverse)
edge_iterator *stack;
edge_iterator ei, einext;
int sp;
- /* Start block (ENTRY_BLOCK_PTR for forward problem, EXIT_BLOCK for backward
+ /* Start block (the entry block for forward problem, exit block for backward
problem). */
basic_block en_block;
/* Ending block. */
diff --git a/gcc/function.c b/gcc/function.c
index fde4a8e..5b33c46 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -6349,7 +6349,7 @@ thread_prologue_and_epilogue_insns (void)
{
unsigned i, last;
- /* convert_jumps_to_returns may add to EXIT_BLOCK_PTR->preds
+ /* convert_jumps_to_returns may add to preds of the exit block
(but won't remove). Stop at end of current preds. */
last = EDGE_COUNT (EXIT_BLOCK_PTR_FOR_FN (cfun)->preds);
for (i = 0; i < last; i++)
diff --git a/gcc/graphite-scop-detection.c b/gcc/graphite-scop-detection.c
index 0cfb5a5..15c4c0f 100644
--- a/gcc/graphite-scop-detection.c
+++ b/gcc/graphite-scop-detection.c
@@ -518,7 +518,7 @@ scopdet_basic_block_info (basic_block bb, loop_p outermost_loop,
result.next = exit_e->dest;
/* If we do not dominate result.next, remove it. It's either
- the EXIT_BLOCK_PTR, or another bb dominates it and will
+ the exit block, or another bb dominates it and will
call the scop detection for this bb. */
if (!dominated_by_p (CDI_DOMINATORS, result.next, bb))
result.next = NULL;
diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
index d7d6b8f..2e8a062 100644
--- a/gcc/ipa-split.c
+++ b/gcc/ipa-split.c
@@ -635,7 +635,7 @@ consider_split (struct split_point *current, bitmap non_ssa_vars,
<retval> = tmp_var;
return <retval>
but return_bb can not be more complex than this.
- If nothing is found, return EXIT_BLOCK_PTR.
+ If nothing is found, return the exit block.
When there are multiple RETURN statement, chose one with return value,
since that one is more likely shared by multiple code paths.
diff --git a/gcc/profile.c b/gcc/profile.c
index 85671b3..1d0e78a 100644
--- a/gcc/profile.c
+++ b/gcc/profile.c
@@ -1392,7 +1392,7 @@ find_spanning_tree (struct edge_list *el)
union_groups (EXIT_BLOCK_PTR_FOR_FN (cfun), ENTRY_BLOCK_PTR_FOR_FN (cfun));
/* First add all abnormal edges to the tree unless they form a cycle. Also
- add all edges to EXIT_BLOCK_PTR to avoid inserting profiling code behind
+ add all edges to the exit block to avoid inserting profiling code behind
setting return value from function. */
for (i = 0; i < num_edges; i++)
{
diff --git a/gcc/sched-int.h b/gcc/sched-int.h
index 070404c..84b5cb5 100644
--- a/gcc/sched-int.h
+++ b/gcc/sched-int.h
@@ -70,7 +70,7 @@ struct common_sched_info_def
/* Called to notify frontend, that new basic block is being added.
The first parameter - new basic block.
The second parameter - block, after which new basic block is being added,
- or EXIT_BLOCK_PTR, if recovery block is being added,
+ or the exit block, if recovery block is being added,
or NULL, if standalone block is being added. */
void (*add_block) (basic_block, basic_block);
diff --git a/gcc/tree-cfgcleanup.c b/gcc/tree-cfgcleanup.c
index 4e5adc2..5ae70ab 100644
--- a/gcc/tree-cfgcleanup.c
+++ b/gcc/tree-cfgcleanup.c
@@ -237,7 +237,7 @@ cleanup_control_flow_bb (basic_block bb)
the start of the successor block.
As a precondition, we require that BB be not equal to
- ENTRY_BLOCK_PTR. */
+ the entry block. */
static bool
tree_forwarder_block_p (basic_block bb, bool phi_wanted)
@@ -250,7 +250,7 @@ tree_forwarder_block_p (basic_block bb, bool phi_wanted)
/* If PHI_WANTED is false, BB must not have any PHI nodes.
Otherwise, BB must have PHI nodes. */
|| gimple_seq_empty_p (phi_nodes (bb)) == phi_wanted
- /* BB may not be a predecessor of EXIT_BLOCK_PTR. */
+ /* BB may not be a predecessor of the exit block. */
|| single_succ (bb) == EXIT_BLOCK_PTR_FOR_FN (cfun)
/* Nor should this be an infinite loop. */
|| single_succ (bb) == bb
diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 25705a9..e9ddb16 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -2433,9 +2433,10 @@ copy_cfg_body (copy_body_data * id, gcov_type count, int frequency_scale,
/* Register specific tree functions. */
gimple_register_cfg_hooks ();
- /* If we are inlining just region of the function, make sure to connect new entry
- to ENTRY_BLOCK_PTR. Since new entry can be part of loop, we must compute
- frequency and probability of ENTRY_BLOCK_PTR based on the frequencies and
+ /* If we are inlining just region of the function, make sure to connect
+ new entry to ENTRY_BLOCK_PTR_FOR_FN (cfun). Since new entry can be
+ part of loop, we must compute frequency and probability of
+ ENTRY_BLOCK_PTR_FOR_FN (cfun) based on the frequencies and
probabilities of edges incoming from nonduplicated region. */
if (new_entry)
{
diff --git a/gcc/tree-scalar-evolution.h b/gcc/tree-scalar-evolution.h
index 8846fbe..fc87251 100644
--- a/gcc/tree-scalar-evolution.h
+++ b/gcc/tree-scalar-evolution.h
@@ -40,8 +40,8 @@ extern bool simple_iv (struct loop *, struct loop *, tree, struct affine_iv_d *,
bool);
extern tree compute_overall_effect_of_inner_loop (struct loop *, tree);
-/* Returns the basic block preceding LOOP or ENTRY_BLOCK_PTR when the
- loop is function's body. */
+/* Returns the basic block preceding LOOP, or the CFG entry block when
+ the loop is function's body. */
static inline basic_block
block_before_loop (loop_p loop)
diff --git a/gcc/tree-ssa-propagate.c b/gcc/tree-ssa-propagate.c
index b9db34c5..b45ff47 100644
--- a/gcc/tree-ssa-propagate.c
+++ b/gcc/tree-ssa-propagate.c
@@ -503,7 +503,7 @@ ssa_prop_init (void)
cfg_blocks.safe_grow_cleared (20);
/* Initially assume that every edge in the CFG is not executable.
- (including the edges coming out of ENTRY_BLOCK_PTR). */
+ (including the edges coming out of the entry block). */
FOR_ALL_BB (bb)
{
gimple_stmt_iterator si;