Patchwork [RFC] Avoid excessive BLOCK associations for locations

login
register
mail settings
Submitter Richard Guenther
Date Jan. 28, 2013, 2:15 p.m.
Message ID <alpine.LNX.2.00.1301281508330.6889@zhemvz.fhfr.qr>
Download mbox | patch
Permalink /patch/216216/
State New
Headers show

Comments

Richard Guenther - Jan. 28, 2013, 2:15 p.m.
This avoids assigning BLOCKs to things that didn't have one before
(originally I observed that the code snippets below happily generate
a UNKNOWN_LOCATION, id->block association).  A previous patch last
year changed expansion in a way to not jump back to the outermost
block when observing a NULL LOCATION_BLOCK in the IL, but similar
to UNKNOWN_LOCATION locus handling just inherit the currently active
BLOCK.  Thus the patch below, instead of just avoiding the non-sensical
UNKNOWN_LOCATION, id->block association goes one step further and
never puts things in the outermost inline BLOCK if it didn't have
a BLOCK assigned before.  This avoids the original non-sensical
issue and avoids excessive BLOCK associations where they are of not
much use.

What's the point of switching to the outermost scope for unknown-BLOCK
locations?  Isn't inheriting the currently active scope much more
useful (it definitely is for UNKNOWN_LOCATIONs)?  If we have a
non-UNKNOWN_LOCATION, would a NULL BLOCK not be an error anyway?
An error we "hide" in the current scheme?

Bootstrapped and tested on x86_64-unknown-linux-gnu.

Does this make sense?

Thanks,
Richard.

2013-01-28  Richard Biener  <rguenther@suse.de>

	* tree-inline.c (remap_gimple_stmt): Do not assing a BLOCK
	to a stmt that didn't have one.
	(copy_phis_for_bb): Likewise for PHI arguments.
	(copy_debug_stmt): Likewise for debug stmts.
Jakub Jelinek - Jan. 28, 2013, 2:37 p.m.
On Mon, Jan 28, 2013 at 03:15:53PM +0100, Richard Biener wrote:
> Does this make sense?

Yes.  Wouldn't hurt to run GDB testsuite with that, though I bet most of it
is -O0 anyway and thus won't stress it out too much.

> 2013-01-28  Richard Biener  <rguenther@suse.de>
> 
> 	* tree-inline.c (remap_gimple_stmt): Do not assing a BLOCK
> 	to a stmt that didn't have one.
> 	(copy_phis_for_bb): Likewise for PHI arguments.
> 	(copy_debug_stmt): Likewise for debug stmts.

Ok.

	Jakub
Michael Matz - Jan. 28, 2013, 3:01 p.m.
Hi,

On Mon, 28 Jan 2013, Richard Biener wrote:

> What's the point of switching to the outermost scope for unknown-BLOCK
> locations?

It's the most sensical block for code which isn't otherwise associated 
with a BLOCK.  But the latter Shouldn't Happen, because conceptually all 
code runs in some (perhaps artificial) lookup context.  So, it's actually 
not the inliner which should fixup stuff here, but rather ...

> If we have a non-UNKNOWN_LOCATION, would a NULL BLOCK not be an error 
> anyway?

... whatever is producing such non-BLOCK code snippets.  But see below.

> Isn't inheriting the currently active scope much more useful (it 
> definitely is for UNKNOWN_LOCATIONs)?

And yes, the most likely useful block for such code will be the "currently 
active" block.  This is true only before code transformations of course; 
while optimizing you have the same problems like with locations, i.e. how 
to "merge" multiple different BLOCKs into one sensible.

Now, as an implementation optimization (to not bloat the location/block 
sets perhaps) you can define block==NULL <--> block==outermost-scope, and 
in case you do so, it's indeed the inliner that needs to map NULL blocks 
to the mapped outermost scope of the inlined function.  I would guess that 
this is what historically was done, and when this optimization is still 
employed your patch is wrong.  IMHO this optimization should be used.


Ciao,
Michael.

Patch

Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c	(revision 195502)
+++ gcc/tree-inline.c	(working copy)
@@ -1198,7 +1198,6 @@  remap_gimple_stmt (gimple stmt, copy_bod
 {
   gimple copy = NULL;
   struct walk_stmt_info wi;
-  tree new_block;
   bool skip_first = false;
 
   /* Begin by recognizing trees that we'll completely rewrite for the
@@ -1458,19 +1457,15 @@  remap_gimple_stmt (gimple stmt, copy_bod
     }
 
   /* If STMT has a block defined, map it to the newly constructed
-     block.  When inlining we want statements without a block to
-     appear in the block of the function call.  */
-  new_block = id->block;
+     block.  */
   if (gimple_block (copy))
     {
       tree *n;
       n = (tree *) pointer_map_contains (id->decl_map, gimple_block (copy));
       gcc_assert (n);
-      new_block = *n;
+      gimple_set_block (copy, *n);
     }
 
-  gimple_set_block (copy, new_block);
-
   if (gimple_debug_bind_p (copy) || gimple_debug_source_bind_p (copy))
     return copy;
 
@@ -1987,7 +1982,6 @@  copy_phis_for_bb (basic_block bb, copy_b
 	      edge old_edge = find_edge ((basic_block) new_edge->src->aux, bb);
 	      tree arg;
 	      tree new_arg;
-	      tree block = id->block;
 	      edge_iterator ei2;
 	      location_t locus;
 
@@ -2015,19 +2009,18 @@  copy_phis_for_bb (basic_block bb, copy_b
 		  inserted = true;
 		}
 	      locus = gimple_phi_arg_location_from_edge (phi, old_edge);
-	      block = id->block;
 	      if (LOCATION_BLOCK (locus))
 		{
 		  tree *n;
 		  n = (tree *) pointer_map_contains (id->decl_map,
 			LOCATION_BLOCK (locus));
 		  gcc_assert (n);
-		  block = *n;
+		  locus = COMBINE_LOCATION_DATA (line_table, locus, *n);
 		}
+	      else
+		locus = LOCATION_LOCUS (locus);
 
-	      add_phi_arg (new_phi, new_arg, new_edge, block ?
-		  COMBINE_LOCATION_DATA (line_table, locus, block) :
-		  LOCATION_LOCUS (locus));
+	      add_phi_arg (new_phi, new_arg, new_edge, locus);
 	    }
 	}
     }
@@ -2324,14 +2317,11 @@  copy_debug_stmt (gimple stmt, copy_body_
   tree t, *n;
   struct walk_stmt_info wi;
 
-  t = id->block;
   if (gimple_block (stmt))
     {
       n = (tree *) pointer_map_contains (id->decl_map, gimple_block (stmt));
-      if (n)
-	t = *n;
+      gimple_set_block (stmt, n ? *n : id->block);
     }
-  gimple_set_block (stmt, t);
 
   /* Remap all the operands in COPY.  */
   memset (&wi, 0, sizeof (wi));