Patchwork instrument clones

login
register
mail settings
Submitter Aldy Hernandez
Date Nov. 16, 2012, 9:28 p.m.
Message ID <50A6AFE1.9060103@redhat.com>
Download mbox | patch
Permalink /patch/199734/
State New
Headers show

Comments

Aldy Hernandez - Nov. 16, 2012, 9:28 p.m.
The included small change to g++.dg/tm/pr51516.C fixes the remaining TM 
regression.  With two code paths, there are two instances of the 
constructor (a clone and an uninstrumented version), so that part of the 
test is no longer relevant.

However... in fixing this, I noticed that for some odd reason we are not 
instrumenting clones at all.  I have no idea, how we missed this, but if 
you look at the (currently) generated code for:

__attribute__((transaction_callable))
void cloneme()
{
   foo = 666;
}

...you will notice that the clone version has no instrumentation.

This is because the iteration with get_bb_regions_uninstrumented() exits 
if there are no exit_blocks, which is always the case for TM clones.  I 
added a new parameter so we can force traversal of the clone if we are 
looking at a clone.

But then this is problematic if the clone has inline assembly because 
collect_bb2reg() does not stop at irrevocable blocks.  So if we 
indiscriminately scan clones, we plow right through irrevocable blocks 
and try to incorrectly instrument them.  Fixed with yet another parameter.

With this patch we have no TM regressions whatsoever, and as a bonus we 
are instrumenting clones :-).

OK?
commit 92723b5bb9d6791b1f3466e5106db13f143da2ca
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Fri Nov 16 15:12:47 2012 -0600

    	* trans-mem (collect_bb2reg): Stop scanning at irrevocable
    	* blocks.
    	(get_bb_regions_instrumented): Add new traverse_clone argument and
    	use it.
    	(expand_regions_1): Same.
    	(expand_region): Same.
    	(execute_tm_mark): Pass new argument to expand_regions.
    	(expand_block_edges): Pass new argument to
    	get_bb_regions_instrumented.
    testsuite/
    	* g++.dg/tm/pr51516.C: Adjust for uninstrumented code path.
    	* gcc.dg/tm/clone-1.c: New test.
Richard Henderson - Nov. 17, 2012, 6:58 p.m.
On 11/16/2012 01:28 PM, Aldy Hernandez wrote:
>     	* trans-mem (collect_bb2reg): Stop scanning at irrevocable
>     	* blocks.
>     	(get_bb_regions_instrumented): Add new traverse_clone argument and
>     	use it.
>     	(expand_regions_1): Same.
>     	(expand_region): Same.
>     	(execute_tm_mark): Pass new argument to expand_regions.
>     	(expand_block_edges): Pass new argument to
>     	get_bb_regions_instrumented.
>     testsuite/
>     	* g++.dg/tm/pr51516.C: Adjust for uninstrumented code path.
>     	* gcc.dg/tm/clone-1.c: New test.

Ok.



r~

Patch

diff --git a/gcc/testsuite/g++.dg/tm/pr51516.C b/gcc/testsuite/g++.dg/tm/pr51516.C
index c13ae47..4e91006 100644
--- a/gcc/testsuite/g++.dg/tm/pr51516.C
+++ b/gcc/testsuite/g++.dg/tm/pr51516.C
@@ -18,5 +18,4 @@  int main()
 }
 
 /* { dg-final { scan-assembler-not "_ITM_getTMCloneOrIrrevocable" } } */
-/* { dg-final { scan-tree-dump-times ";; Function C::C" 1 "optimized" } } */
 /* { dg-final { cleanup-tree-dump "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/tm/clone-1.c b/gcc/testsuite/gcc.dg/tm/clone-1.c
new file mode 100644
index 0000000..4050add
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tm/clone-1.c
@@ -0,0 +1,13 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fgnu-tm -fdump-tree-tmmark" } */
+
+int foo;
+
+__attribute__((transaction_callable))
+void cloneme()
+{
+  foo = 666;
+}
+
+/* { dg-final { scan-tree-dump-times "ITM_WU.*foo" 1 "tmmark" } } */
+/* { dg-final { cleanup-tree-dump "tmmark" } } */
diff --git a/gcc/trans-mem.c b/gcc/trans-mem.c
index a7b4a9c..14d0ca9 100644
--- a/gcc/trans-mem.c
+++ b/gcc/trans-mem.c
@@ -138,7 +138,7 @@ 
 
 static void *expand_regions (struct tm_region *,
 			     void *(*callback)(struct tm_region *, void *),
-			     void *);
+			     void *, bool);
 
 
 /* Return the attributes we want to examine for X, or NULL if it's not
@@ -2457,7 +2457,7 @@  collect_bb2reg (struct tm_region *region, void *data)
 				region->exit_blocks,
 				region->irr_blocks,
 				NULL,
-				/*stop_at_irr_p=*/false);
+				/*stop_at_irr_p=*/true);
 
   // We expect expand_region to perform a post-order traversal of the region
   // tree.  Therefore the last region seen for any bb is the innermost.
@@ -2491,14 +2491,14 @@  collect_bb2reg (struct tm_region *region, void *data)
 // only known instance of this block sharing.
 
 static VEC(tm_region_p, heap) *
-get_bb_regions_instrumented (void)
+get_bb_regions_instrumented (bool traverse_clones)
 {
   unsigned n = last_basic_block;
   VEC(tm_region_p, heap) *ret;
 
   ret = VEC_alloc (tm_region_p, heap, n);
   VEC_safe_grow_cleared (tm_region_p, heap, ret, n);
-  expand_regions (all_tm_regions, collect_bb2reg, ret);
+  expand_regions (all_tm_regions, collect_bb2reg, ret, traverse_clones);
 
   return ret;
 }
@@ -2826,11 +2826,13 @@  execute_tm_mark (void)
 {
   pending_edge_inserts_p = false;
 
-  expand_regions (all_tm_regions, generate_tm_state, NULL);
+  expand_regions (all_tm_regions, generate_tm_state, NULL,
+		  /*traverse_clones=*/true);
 
   tm_log_init ();
 
-  VEC(tm_region_p, heap) *bb_regions = get_bb_regions_instrumented ();
+  VEC(tm_region_p, heap) *bb_regions
+    = get_bb_regions_instrumented (/*traverse_clones=*/true);
   struct tm_region *r;
   unsigned i;
 
@@ -2844,7 +2846,8 @@  execute_tm_mark (void)
   propagate_tm_flags_out (all_tm_regions);
 
   // Expand GIMPLE_TRANSACTIONs into calls into the runtime.
-  expand_regions (all_tm_regions, expand_transaction, NULL);
+  expand_regions (all_tm_regions, expand_transaction, NULL,
+		  /*traverse_clones=*/false);
 
   tm_log_emit ();
   tm_log_delete ();
@@ -3000,7 +3003,8 @@  expand_block_edges (struct tm_region *const region, basic_block bb)
 static unsigned int
 execute_tm_edges (void)
 {
-  VEC(tm_region_p, heap) *bb_regions = get_bb_regions_instrumented ();
+  VEC(tm_region_p, heap) *bb_regions
+    = get_bb_regions_instrumented (/*traverse_clones=*/false);
   struct tm_region *r;
   unsigned i;
 
@@ -3044,15 +3048,18 @@  struct gimple_opt_pass pass_tm_edges =
 /* Helper function for expand_regions.  Expand REGION and recurse to
    the inner region.  Call CALLBACK on each region.  CALLBACK returns
    NULL to continue the traversal, otherwise a non-null value which
-   this function will return as well.  */
+   this function will return as well.  TRAVERSE_CLONES is true if we
+   should traverse transactional clones.  */
 
 static void *
 expand_regions_1 (struct tm_region *region,
 		  void *(*callback)(struct tm_region *, void *),
-		  void *data)
+		  void *data,
+		  bool traverse_clones)
 {
   void *retval = NULL;
-  if (region->exit_blocks)
+  if (region->exit_blocks
+      || (traverse_clones && decl_is_tm_clone (current_function_decl)))
     {
       retval = callback (region, data);
       if (retval)
@@ -3060,7 +3067,7 @@  expand_regions_1 (struct tm_region *region,
     }
   if (region->inner)
     {
-      retval = expand_regions (region->inner, callback, data);
+      retval = expand_regions (region->inner, callback, data, traverse_clones);
       if (retval)
 	return retval;
     }
@@ -3070,17 +3077,19 @@  expand_regions_1 (struct tm_region *region,
 /* Traverse the regions enclosed and including REGION.  Execute
    CALLBACK for each region, passing DATA.  CALLBACK returns NULL to
    continue the traversal, otherwise a non-null value which this
-   function will return as well.  */
+   function will return as well.  TRAVERSE_CLONES is true if we should
+   traverse transactional clones.  */
 
 static void *
 expand_regions (struct tm_region *region,
 		void *(*callback)(struct tm_region *, void *),
-		void *data)
+		void *data,
+		bool traverse_clones)
 {
   void *retval = NULL;
   while (region)
     {
-      retval = expand_regions_1 (region, callback, data);
+      retval = expand_regions_1 (region, callback, data, traverse_clones);
       if (retval)
 	return retval;
       region = region->next;