diff mbox

[RFC] Avoid excessive BLOCK associations for locations

Message ID alpine.LNX.2.00.1301281508330.6889@zhemvz.fhfr.qr
State New
Headers show

Commit Message

Richard Biener Jan. 28, 2013, 2:15 p.m. UTC 
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.

Comments

Jakub Jelinek Jan. 28, 2013, 2:37 p.m. UTC | #1 
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. UTC | #2 
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.
diff mbox

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