[buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR

[buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR

[Jan-Benedict Glaw]

[-- Attachment #1: Type: text/plain, Size: 4341 bytes --]

Hi!

One usage of ENTRY_BLOCK_PTR was missed:

g++ -c   -g -O2 -DIN_GCC  -DCROSS_DIRECTORY_STRUCTURE  -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wwrite-strings -Wcast-qual -Wmissing-format-attribute -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -fno-common  -DHAVE_CONFIG_H -I. -I. -I/home/jbglaw/repos/gcc/gcc -I/home/jbglaw/repos/gcc/gcc/. -I/home/jbglaw/repos/gcc/gcc/../include -I/home/jbglaw/repos/gcc/gcc/../libcpp/include  -I/home/jbglaw/repos/gcc/gcc/../libdecnumber -I/home/jbglaw/repos/gcc/gcc/../libdecnumber/dpd -I../libdecnumber -I/home/jbglaw/repos/gcc/gcc/../libbacktrace    -o mips.o -MT mips.o -MMD -MP -MF ./.deps/mips.TPo /home/jbglaw/repos/gcc/gcc/config/mips/mips.c
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c: In function ‘void mips_insert_attributes(tree_node*, tree_node**)’:
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c:1403: warning: unknown conversion type character ‘E’ in format
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c:1403: warning: format ‘%qs’ expects type ‘char*’, but argument 2 has type ‘tree_node*’
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c:1403: warning: too many arguments for format
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c:1408: warning: unknown conversion type character ‘E’ in format
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c:1408: warning: format ‘%qs’ expects type ‘char*’, but argument 2 has type ‘tree_node*’
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c:1408: warning: too many arguments for format
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c: In function ‘tree_node* mips_merge_decl_attributes(tree_node*, tree_node*)’:
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c:1437: warning: unknown conversion type character ‘E’ in format
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c:1437: warning: format ‘%qs’ expects type ‘char*’, but argument 2 has type ‘tree_node*’
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c:1437: warning: too many arguments for format
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c:1443: warning: unknown conversion type character ‘E’ in format
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c:1443: warning: format ‘%qs’ expects type ‘char*’, but argument 2 has type ‘tree_node*’
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c:1443: warning: too many arguments for format
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c: In function ‘rtx_def* mips_expand_builtin(tree_node*, rtx_def*, rtx_def*, machine_mode, int)’:
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c:14458: warning: unknown conversion type character ‘E’ in format
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c:14458: warning: too many arguments for format
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c: In function ‘rtx_def* r10k_simplify_address(rtx_def*, rtx_def*)’:
/home/jbglaw/repos/gcc/gcc/config/mips/mips.c:14845: error: ‘ENTRY_BLOCK_PTR’ was not declared in this scope
make[1]: *** [mips.o] Error 1

(http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=36303)

This should fix it:

2013-11-20  Jan-Benedict Glaw  <jbglaw@lug-owl.de>

	* config/mips/mips.c (r10k_simplify_address): Eliminate macro usage.

diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 82ca719..d06d574 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -14842,7 +14842,7 @@ r10k_simplify_address (rtx x, rtx insn)
 	      /* Replace the incoming value of $sp with
 		 virtual_incoming_args_rtx.  */
 	      if (x == stack_pointer_rtx
-		  && DF_REF_BB (def) == ENTRY_BLOCK_PTR)
+		  && DF_REF_BB (def) == ENTRY_BLOCK_PTR_FOR_FN (cfun))
 		newx = virtual_incoming_args_rtx;
 	    }
 	  else if (dominated_by_p (CDI_DOMINATORS, DF_REF_BB (use),


Ok?

MfG, JBG

-- 
      Jan-Benedict Glaw      jbglaw@lug-owl.de              +49-172-7608481
Signature of: 23:53 <@jbglaw> So, ich kletter' jetzt mal ins Bett.
the second  : 23:57 <@jever2> .oO( kletter ..., hat er noch Gitter vorm Bett, wie früher meine Kinder?)
              00:00 <@jbglaw> jever2: *patsch*
              00:01 <@jever2> *aua*, wofür, Gedanken sind frei!
              00:02 <@jbglaw> Nee, freie Gedanken, die sind seit 1984 doch aus!
              00:03 <@jever2> 1984? ich bin erst seit 1985 verheiratet!

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR

[Richard Sandiford]

Jan-Benedict Glaw <jbglaw@lug-owl.de> writes:
> 2013-11-20  Jan-Benedict Glaw  <jbglaw@lug-owl.de>
>
> 	* config/mips/mips.c (r10k_simplify_address): Eliminate macro usage.

OK.  And thanks for the catch.

Richard

Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR

[Steven Bosscher]

On Wed, Nov 20, 2013 at 10:04 AM, Jan-Benedict Glaw wrote:
> 2013-11-20  Jan-Benedict Glaw  <...>
>
>         * config/mips/mips.c (r10k_simplify_address): Eliminate macro usage.
>
> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
> index 82ca719..d06d574 100644
> --- a/gcc/config/mips/mips.c
> +++ b/gcc/config/mips/mips.c
> @@ -14842,7 +14842,7 @@ r10k_simplify_address (rtx x, rtx insn)
>               /* Replace the incoming value of $sp with
>                  virtual_incoming_args_rtx.  */
>               if (x == stack_pointer_rtx
> -                 && DF_REF_BB (def) == ENTRY_BLOCK_PTR)
> +                 && DF_REF_BB (def) == ENTRY_BLOCK_PTR_FOR_FN (cfun))
>                 newx = virtual_incoming_args_rtx;
>             }
>           else if (dominated_by_p (CDI_DOMINATORS, DF_REF_BB (use),
>
>
> Ok?
>
> MfG, JBG

This patch is obvious and it fixes breakage. Please go ahead and commit it.

I wonder if there are any more cases like this missed... Could you
please check that? Something like:

egrep -w "ENTRY_BLOCK_PTR|EXIT_BLOCK_PTR" gcc/*.[ch] gcc/config/*.[ch]
gcc/config/*/*.{c,h,md}

should do the trick. (I'd do it myself if I had access to a Linux box
right now...)

Ciao!
Steven

Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR

[Jan-Benedict Glaw]

[-- Attachment #1: Type: text/plain, Size: 562 bytes --]

On Wed, 2013-11-20 10:08:45 +0100, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
[...]
> I wonder if there are any more cases like this missed... Could you
> please check that? Something like:
> 
> egrep -w "ENTRY_BLOCK_PTR|EXIT_BLOCK_PTR" gcc/*.[ch] gcc/config/*.[ch]
> gcc/config/*/*.{c,h,md}

No more uses, but 21 comment lines contain references to the macros.

MfG, JBG

-- 
      Jan-Benedict Glaw      jbglaw@lug-owl.de              +49-172-7608481
Signature of:                 Friends are relatives you make for yourself.
the second  :

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[David Malcolm]

[-- Attachment #1: Type: text/plain, Size: 2195 bytes --]

On Wed, 2013-11-20 at 11:07 +0100, Jan-Benedict Glaw wrote:
> On Wed, 2013-11-20 10:08:45 +0100, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> [...]
> > I wonder if there are any more cases like this missed... Could you
> > please check that? Something like:
> > 
> > egrep -w "ENTRY_BLOCK_PTR|EXIT_BLOCK_PTR" gcc/*.[ch] gcc/config/*.[ch]
> > gcc/config/*/*.{c,h,md}
> 
> No more uses, but 21 comment lines contain references to the macros.

Sorry about the mips breakage, and thanks for fixing it.

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

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

FWIW the wording of this comment came from r53804:

  2002-05-23  Zdenek Dvorak  <rakdver@atrey.karlin.mff.cuni.cz>

       * bb-reorder.c [...]:  Use FOR_EACH_BB macros to iterate over
basic block chain.
       [...]
       * cfgbuild.c [...]: Likewise.

which made this change to the comment:
-  /* 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.  */

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

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

Comment-fixing patch follows.

[-- Attachment #2: reword-comments.patch --]
[-- Type: text/x-patch, Size: 10093 bytes --]

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;

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

[Jeff Law]

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

Re: [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.

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

[Michael Matz]

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.

gcc's obvious patch policy

[Alan Modra]

Was Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR
On Wed, Nov 20, 2013 at 10:08:45AM +0100, Steven Bosscher wrote:
> This patch is obvious and it fixes breakage. Please go ahead and commit it.

Sorry to pick on you here Steven, but this doesn't meet gcc's
definition of an obvious patch.  Don't believe me?  See
http://gcc.gnu.org/svnwrite.html#policies

Allowed as obvious in the gcc sources are typo fixes for comments or
similar, or reverting a bad patch you made.  That's it.  The power to
change anything else is reserved to the relevant maintainer.

Last I checked, you're not a MIPS maintainer..  Am I rebuking you?
No, not at all!  You just gave me a perfect lead in for the
following..  :)

/rant
It's utterly ridiculous that gcc doesn't have a reasonable obvious
patch rule.  Only comments?  In that all non-maintainers can be
trusted with?  What a poor lot of contributors we have.

Should a maintainer not even be able to authorise simple patches out
of their area as Steven just did?  That's what a strict interpretation
of the current rules in MAINTAINERS plus the current obvious patch
rule implies.  Or does the obvious patch rule just apply to
non-maintainers?
/no-rant

Can I recommend gdb's obvious patch policy?  It even tickles my sense
of humour.  "will the person who hates my work the most be able to
find fault with the change" - if so, then it's not obvious..

-- 
Alan Modra
Australia Development Lab, IBM

Re: gcc's obvious patch policy

[Steven Bosscher]

On Tue, Nov 26, 2013 at 6:17 AM, Alan Modra wrote:
> Was Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR
> On Wed, Nov 20, 2013 at 10:08:45AM +0100, Steven Bosscher wrote:
>> This patch is obvious and it fixes breakage. Please go ahead and commit it.
>
> Sorry to pick on you here Steven, but this doesn't meet gcc's
> definition of an obvious patch.  Don't believe me?  See
> http://gcc.gnu.org/svnwrite.html#policies

Hmm.... I guess the patch will have to be reverted, then :-)

Or maybe this would be under the banner of "We don't want to get
overly anal-retentive about checkin policies."

In any case, it's not unprecedented that obviously obvious patches get
checked in even if they're not obvious according to that policy. To
list a few from just this month:

http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02989.html
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02975.html
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02970.html
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02972.html
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02496.html
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02331.html

So perhaps the policy should include a line about fixing trivial
breakage from recent check-ins.

Ciao!
Steven

Re: gcc's obvious patch policy

[David Edelsohn]

>> Sorry to pick on you here Steven, but this doesn't meet gcc's
>> definition of an obvious patch.  Don't believe me?

> No.  I don't, let me quote from the policy:

I find this whole thread a rather sad and pathetic bikeshed
discussion. Regardless of the formal policy, the basic concept is to
use common sense.  Common sense about the context of the code being
changed, common sense about the patch itself, and common sense about
the maintenance area and the maintainers.

Anything more than that is people trying to create / change rules as a
stick to hit each other over the head or a straight jacket to tie each
other up.

Thanks, David

Re: gcc's obvious patch policy

[Robert Dewar]

To me the issue is not what is written down about
the policy, but whether the policy works in practice,
and it seems like it does, so what's the problem?

This just seems to be making a problem where
none exists.

Re: gcc's obvious patch policy

[Alan Modra]

On Tue, Nov 26, 2013 at 04:56:26PM -0500, Robert Dewar wrote:
> To me the issue is not what is written down about
> the policy, but whether the policy works in practice,
> and it seems like it does, so what's the problem?
> 
> This just seems to be making a problem where
> none exists.

I gave some background in my email to David over why I'm stirring the
pot here.

The thing about written policy is that it sets the tone for a project.
A restrictive policy tends to authoritarian rule by maintainers, it
seems to me.  A less restricive policy ought to ease some of the
nonsense that goes on currently, for instance, port maintainers
thinking they need to get global maintainer permission for trivial
patches outside their area of responsibility.  As an example, for
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02793.html I was told
I'd need global maintainer approval..  Well, maybe I do in the current
climate.

I hope I haven't offended the review gods too much here.  I'm sure
other people have noticed the issues I'm raising but have more wisely
than I, kept quiet.  

-- 
Alan Modra
Australia Development Lab, IBM

Re: gcc's obvious patch policy

[Richard Kenner]

> The thing about written policy is that it sets the tone for a project.
> A restrictive policy tends to authoritarian rule by maintainers, it
> seems to me. 

And a too little restrictive policy runs the risk of creating a
feeling that the rules aren't necessarily to be taken too seriously.
Neither outcome is good.

Re: gcc's obvious patch policy

[Alan Modra]

On Tue, Nov 26, 2013 at 04:30:50PM -0500, David Edelsohn wrote:
> >> Sorry to pick on you here Steven, but this doesn't meet gcc's
> >> definition of an obvious patch.  Don't believe me?
> 
> > No.  I don't, let me quote from the policy:
> 
> I find this whole thread a rather sad and pathetic bikeshed
> discussion. Regardless of the formal policy, the basic concept is to
> use common sense.  Common sense about the context of the code being
> changed, common sense about the patch itself, and common sense about
> the maintenance area and the maintainers.
> 
> Anything more than that is people trying to create / change rules as a
> stick to hit each other over the head or a straight jacket to tie each
> other up.

I find this a bit rich coming from you, David.  On the weekend I
committed a patch as obvious, for which you "hit me over the head",
stating in no uncertain terms that I should not bypass you and commit
patches like that as "obvious".  I still think the substance of the
patch was obvious for anyone who has worked on the powerpc backend for
as long as I have, but after some discussion I backed down because
technically, you were within your rights and I had transgressed the
rules.

You have the stick *now*.  And wield it.  I'm trying to take it away
from you..

-- 
Alan Modra
Australia Development Lab, IBM

Re: gcc's obvious patch policy

[David Edelsohn]

On Tue, Nov 26, 2013 at 8:37 PM, Alan Modra <amodra@gmail.com> wrote:

>> I find this whole thread a rather sad and pathetic bikeshed
>> discussion. Regardless of the formal policy, the basic concept is to
>> use common sense.  Common sense about the context of the code being
>> changed, common sense about the patch itself, and common sense about
>> the maintenance area and the maintainers.
>>
>> Anything more than that is people trying to create / change rules as a
>> stick to hit each other over the head or a straight jacket to tie each
>> other up.
>
> I find this a bit rich coming from you, David.  On the weekend I
> committed a patch as obvious, for which you "hit me over the head",
> stating in no uncertain terms that I should not bypass you and commit
> patches like that as "obvious".  I still think the substance of the
> patch was obvious for anyone who has worked on the powerpc backend for
> as long as I have, but after some discussion I backed down because
> technically, you were within your rights and I had transgressed the
> rules.
>
> You have the stick *now*.  And wield it.  I'm trying to take it away
> from you..

Alan,

I privately asked you not to commit obvious patches to the port
because there was no reason to rush the patch as "obvious". There are
a lot of changes happening to the port from many directions and I am
trying to prevent unintended conflict from destabilizing the port.

You're the one turning this into a public issue and making it personal.

- David

Re: gcc's obvious patch policy

[Diego Novillo]

On Tue, Nov 26, 2013 at 12:17 AM, Alan Modra <amodra@gmail.com> wrote:
> Was Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR
> On Wed, Nov 20, 2013 at 10:08:45AM +0100, Steven Bosscher wrote:
>> This patch is obvious and it fixes breakage. Please go ahead and commit it.
>
> Sorry to pick on you here Steven, but this doesn't meet gcc's
> definition of an obvious patch.  Don't believe me?  See
> http://gcc.gnu.org/svnwrite.html#policies
>
> Allowed as obvious in the gcc sources are typo fixes for comments or
> similar, or reverting a bad patch you made.  That's it.  The power to
> change anything else is reserved to the relevant maintainer.

Huh.  That's silly.  It allows nothing interesting!

> Can I recommend gdb's obvious patch policy?  It even tickles my sense
> of humour.  "will the person who hates my work the most be able to
> find fault with the change" - if so, then it's not obvious..

I like this one much better.  Anyone else opposed to changing the
obvious-commit policy to something along these lines?


Diego.

Re: gcc's obvious patch policy

[Jeff Law]

On 11/26/13 08:21, Diego Novillo wrote:
> On Tue, Nov 26, 2013 at 12:17 AM, Alan Modra <amodra@gmail.com> wrote:
>> Was Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR
>> On Wed, Nov 20, 2013 at 10:08:45AM +0100, Steven Bosscher wrote:
>>> This patch is obvious and it fixes breakage. Please go ahead and commit it.
>>
>> Sorry to pick on you here Steven, but this doesn't meet gcc's
>> definition of an obvious patch.  Don't believe me?  See
>> http://gcc.gnu.org/svnwrite.html#policies
>>
>> Allowed as obvious in the gcc sources are typo fixes for comments or
>> similar, or reverting a bad patch you made.  That's it.  The power to
>> change anything else is reserved to the relevant maintainer.
>
> Huh.  That's silly.  It allows nothing interesting!
As I've stated within the last few months, I'm certainly open to 
revisiting that policy.  I believe we put that policy in place in circa 
1998 as we started up egcs.

>
>> Can I recommend gdb's obvious patch policy?  It even tickles my sense
>> of humour.  "will the person who hates my work the most be able to
>> find fault with the change" - if so, then it's not obvious..
>
> I like this one much better.  Anyone else opposed to changing the
> obvious-commit policy to something along these lines?
Seems reasonable to me.

jeff

RE: gcc's obvious patch policy

[Iyer, Balaji V]

> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@gcc.gnu.org] On Behalf Of Jeff Law
> Sent: Tuesday, November 26, 2013 11:31 AM
> To: Diego Novillo; Steven Bosscher; gcc-patches
> Subject: Re: gcc's obvious patch policy
> 
> On 11/26/13 08:21, Diego Novillo wrote:
> > On Tue, Nov 26, 2013 at 12:17 AM, Alan Modra <amodra@gmail.com>
> wrote:
> >> Was Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR On
> >> Wed, Nov 20, 2013 at 10:08:45AM +0100, Steven Bosscher wrote:
> >>> This patch is obvious and it fixes breakage. Please go ahead and commit
> it.
> >>
> >> Sorry to pick on you here Steven, but this doesn't meet gcc's
> >> definition of an obvious patch.  Don't believe me?  See
> >> http://gcc.gnu.org/svnwrite.html#policies
> >>
> >> Allowed as obvious in the gcc sources are typo fixes for comments or
> >> similar, or reverting a bad patch you made.  That's it.  The power to
> >> change anything else is reserved to the relevant maintainer.
> >
> > Huh.  That's silly.  It allows nothing interesting!
> As I've stated within the last few months, I'm certainly open to revisiting that
> policy.  I believe we put that policy in place in circa
> 1998 as we started up egcs.
> 
> >
> >> Can I recommend gdb's obvious patch policy?  It even tickles my sense
> >> of humour.  "will the person who hates my work the most be able to
> >> find fault with the change" - if so, then it's not obvious..
> >
> > I like this one much better.  Anyone else opposed to changing the
> > obvious-commit policy to something along these lines?
> Seems reasonable to me.
> 

Can I make a suggestion that if someone is making an "obvious" change (with the exception of changing non-working code (comments, things inside #if 0, etc)), have a patch on the mailing list for 12-24 hrs. before putting it in? Maybe they could say something like, I will check this in by X time  <TIMEZONE> tomorrow since this looks obvious to me. This way if the change hurts someone who is working on a feature in their local machine that is using the existing framework can chime in.

Thanks,

Balaji V. Iyer.

> jeff

Re: gcc's obvious patch policy

[James Greenhalgh]

On Tue, Nov 26, 2013 at 05:14:22PM +0000, Iyer, Balaji V wrote:
> 
> 
> > -----Original Message-----
> > From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> > owner@gcc.gnu.org] On Behalf Of Jeff Law
> > Sent: Tuesday, November 26, 2013 11:31 AM
> > To: Diego Novillo; Steven Bosscher; gcc-patches
> > Subject: Re: gcc's obvious patch policy
> > 
> > On 11/26/13 08:21, Diego Novillo wrote:
> > > On Tue, Nov 26, 2013 at 12:17 AM, Alan Modra <amodra@gmail.com>
> > wrote:
> > >> Was Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR On
> > >> Wed, Nov 20, 2013 at 10:08:45AM +0100, Steven Bosscher wrote:
> > >>> This patch is obvious and it fixes breakage. Please go ahead and commit
> > it.
> > >>
> > >> Sorry to pick on you here Steven, but this doesn't meet gcc's
> > >> definition of an obvious patch.  Don't believe me?  See
> > >> http://gcc.gnu.org/svnwrite.html#policies
> > >>
> > >> Allowed as obvious in the gcc sources are typo fixes for comments or
> > >> similar, or reverting a bad patch you made.  That's it.  The power to
> > >> change anything else is reserved to the relevant maintainer.
> > >
> > > Huh.  That's silly.  It allows nothing interesting!
> > As I've stated within the last few months, I'm certainly open to revisiting that
> > policy.  I believe we put that policy in place in circa
> > 1998 as we started up egcs.
> > 
> > >
> > >> Can I recommend gdb's obvious patch policy?  It even tickles my sense
> > >> of humour.  "will the person who hates my work the most be able to
> > >> find fault with the change" - if so, then it's not obvious..
> > >
> > > I like this one much better.  Anyone else opposed to changing the
> > > obvious-commit policy to something along these lines?
> > Seems reasonable to me.
> > 
> 
> Can I make a suggestion that if someone is making an "obvious" change (with the exception of changing non-working code (comments, things inside #if 0, etc)), have a patch on the mailing list for 12-24 hrs. before putting it in? Maybe they could say something like, I will check this in by X time  <TIMEZONE> tomorrow since this looks obvious to me. This way if the change hurts someone who is working on a feature in their local machine that is using the existing framework can chime in.
> 

This would seem to exclude the most useful type of obvious fixes,
trivial changes which are currently preventing a bootstrap.

What benefit does waiting 24 hours to add a missing 'unsigned'
give?

Surely If the change were likely to impact someone else in a
non-trivial way, it cannot be considered an obvious change and
thus, this rule would not apply.

James

Re: gcc's obvious patch policy

[Eric Botcazou]

> Can I make a suggestion that if someone is making an "obvious" change (with
> the exception of changing non-working code (comments, things inside #if 0,
> etc)), have a patch on the mailing list for 12-24 hrs. before putting it
> in? Maybe they could say something like, I will check this in by X time 
> <TIMEZONE> tomorrow since this looks obvious to me. This way if the change
> hurts someone who is working on a feature in their local machine that is
> using the existing framework can chime in.

I disagree, obvious patches cannot reasonably invalidate the work of others, 
or else they are simply not obvious.  At worst they can privatize a public 
function or remove an unused one, but then it's easy to undo that later.

-- 
Eric Botcazou

RE: gcc's obvious patch policy

[Iyer, Balaji V]

> -----Original Message-----
> From: Eric Botcazou [mailto:ebotcazou@adacore.com]
> Sent: Tuesday, November 26, 2013 12:33 PM
> To: Iyer, Balaji V
> Cc: gcc-patches@gcc.gnu.org; Diego Novillo; Jeff Law; Steven Bosscher
> Subject: Re: gcc's obvious patch policy
> 
> > Can I make a suggestion that if someone is making an "obvious" change
> > (with the exception of changing non-working code (comments, things
> > inside #if 0, etc)), have a patch on the mailing list for 12-24 hrs.
> > before putting it in? Maybe they could say something like, I will
> > check this in by X time <TIMEZONE> tomorrow since this looks obvious
> > to me. This way if the change hurts someone who is working on a
> > feature in their local machine that is using the existing framework can
> chime in.
> 
> I disagree, obvious patches cannot reasonably invalidate the work of others,
> or else they are simply not obvious.  At worst they can privatize a public
> function or remove an unused one, but then it's easy to undo that later.
> 

Those at worst examples you have mentioned is the ones that scare me. Sometimes when a function becomes private, making it public back again is sometimes an uphill battle. I am not saying they shouldn't commit it, but at least give a heads-up.

This being said, I am Ok with either decision.

> --
> Eric Botcazou

Re: gcc's obvious patch policy

[Diego Novillo]

On Tue, Nov 26, 2013 at 12:40 PM, Iyer, Balaji V
<balaji.v.iyer@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Eric Botcazou [mailto:ebotcazou@adacore.com]
>> Sent: Tuesday, November 26, 2013 12:33 PM
>> To: Iyer, Balaji V
>> Cc: gcc-patches@gcc.gnu.org; Diego Novillo; Jeff Law; Steven Bosscher
>> Subject: Re: gcc's obvious patch policy
>>
>> > Can I make a suggestion that if someone is making an "obvious" change
>> > (with the exception of changing non-working code (comments, things
>> > inside #if 0, etc)), have a patch on the mailing list for 12-24 hrs.
>> > before putting it in? Maybe they could say something like, I will
>> > check this in by X time <TIMEZONE> tomorrow since this looks obvious
>> > to me. This way if the change hurts someone who is working on a
>> > feature in their local machine that is using the existing framework can
>> chime in.
>>
>> I disagree, obvious patches cannot reasonably invalidate the work of others,
>> or else they are simply not obvious.  At worst they can privatize a public
>> function or remove an unused one, but then it's easy to undo that later.
>>
>
> Those at worst examples you have mentioned is the ones that scare me. Sometimes when a function becomes private, making it public back
> again is sometimes an uphill battle. I am not saying they shouldn't commit it, but at least give a heads-up.

Nah. Simple patches like that are always easy to pinpoint and address
afterwards. Obvious patches will always be on the small side, after
all.


Diego.

Re: gcc's obvious patch policy

[Richard Earnshaw]

On 26/11/13 17:14, Iyer, Balaji V wrote:
> 
> 
>> -----Original Message-----
>> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
>> owner@gcc.gnu.org] On Behalf Of Jeff Law
>> Sent: Tuesday, November 26, 2013 11:31 AM
>> To: Diego Novillo; Steven Bosscher; gcc-patches
>> Subject: Re: gcc's obvious patch policy
>>
>> On 11/26/13 08:21, Diego Novillo wrote:
>>> On Tue, Nov 26, 2013 at 12:17 AM, Alan Modra <amodra@gmail.com>
>> wrote:
>>>> Was Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR On
>>>> Wed, Nov 20, 2013 at 10:08:45AM +0100, Steven Bosscher wrote:
>>>>> This patch is obvious and it fixes breakage. Please go ahead and commit
>> it.
>>>>
>>>> Sorry to pick on you here Steven, but this doesn't meet gcc's
>>>> definition of an obvious patch.  Don't believe me?  See
>>>> http://gcc.gnu.org/svnwrite.html#policies
>>>>
>>>> Allowed as obvious in the gcc sources are typo fixes for comments or
>>>> similar, or reverting a bad patch you made.  That's it.  The power to
>>>> change anything else is reserved to the relevant maintainer.
>>>
>>> Huh.  That's silly.  It allows nothing interesting!
>> As I've stated within the last few months, I'm certainly open to revisiting that
>> policy.  I believe we put that policy in place in circa
>> 1998 as we started up egcs.
>>
>>>
>>>> Can I recommend gdb's obvious patch policy?  It even tickles my sense
>>>> of humour.  "will the person who hates my work the most be able to
>>>> find fault with the change" - if so, then it's not obvious..
>>>
>>> I like this one much better.  Anyone else opposed to changing the
>>> obvious-commit policy to something along these lines?
>> Seems reasonable to me.
>>
> 
> Can I make a suggestion that if someone is making an "obvious" change (with the exception of changing non-working code (comments, things inside #if 0, etc)), have a patch on the mailing list for 12-24 hrs. before putting it in? Maybe they could say something like, I will check this in by X time  <TIMEZONE> tomorrow since this looks obvious to me. This way if the change hurts someone who is working on a feature in their local machine that is using the existing framework can chime in.
> 

There may be times when that is undesirable.  For example, the obvious
fix to something that prevents the compiler from building.

I think we have enough checks and balances in place.  Anyone
repeatedly/gratuitously abusing the obvious rule is likely to lose their
commit bit pretty quickly.  We're a community that works by
co-operation, so lets co-operate as much as possible.

R.

Re: gcc's obvious patch policy

[Mike Stump]

On Nov 25, 2013, at 9:17 PM, Alan Modra <amodra@gmail.com> wrote:
> Was Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR
> On Wed, Nov 20, 2013 at 10:08:45AM +0100, Steven Bosscher wrote:
>> This patch is obvious and it fixes breakage. Please go ahead and commit it.
> 
> Sorry to pick on you here Steven, but this doesn't meet gcc's
> definition of an obvious patch.  Don't believe me?

No.  I don't, let me quote from the policy:

  We don't want to get overly anal-retentive about checkin policies.

Nit picking the fine lines from a lawyer perspective as to the exact meaning of the word is, is best suited for lawyers and presidents.  We are above that around here.  We work through co-operation and a shared desire to push gcc in the direction of being the best it can be.

The more rules one writes about what is and is not acceptable and the more capriciousness one uses when applying those rules, the worse off I think we are.  I don't favor that direction.  Merely mentioning that a fix that is checked in to resolve a build issue doesn't meet your understanding of the letter of the law, I think is already takes us in a non-desirable direction.  You discourage lots of people by making them have second thoughts about checking in any fix to any build issue.  I personally favor a compiler that builds.

If you have a substantive disagreement as to the exact formation of the patch that was checked in under the obvious rule, I think the right approach is to point out the issues that you have with the patch and if you feel strongly enough that the source tree is better without it, to ask for the person to revert it (or fix it to more your liking).  In the end, we can trust that a reviewer will settle any disagreements.  They can approve the patch as is, insist that it be removed, explain how it should be fixed instead…

If we change the text of the policy, I think we should resist the temptation to make it more strict.  We can explain that patches checked in under the obvious rule are free to be post-checkin reviewed by a reviewer and that review can include any/all of the usual review responses and can include a please revert.  If anything, we should list more categories of what obvious is, or say, including, but not limited to, so that people don't form an overly restrictive view of the policy as you have done.  A example of a category not listed is unbreaking the build.

> It's utterly ridiculous that gcc doesn't have a reasonable obvious
> patch rule.

Maybe we already do, and you never knew it.  :-)  I already have a reasonable obvious patch rule.

> Only comments?

In the parts I review for, the rule isn't restricted to comments.  The history of gcc is replete with examples that are non-comment changes under the obvious rule and the maintainers of those areas that think, at least in part, as I do as well.