re PR middle-end/51752 (trans-mem: publication safety violated)
authorAldy Hernandez <aldyh@redhat.com>
Tue, 28 Feb 2012 20:08:39 +0000 (20:08 +0000)
committerAldy Hernandez <aldyh@gcc.gnu.org>
Tue, 28 Feb 2012 20:08:39 +0000 (20:08 +0000)
        PR middle-end/51752
        * gimple.h (gimple_in_transaction): New.
        (gimple_set_in_transaction): New.
        (struct gimple_statement_base): Add in_transaction field.
        * tree-ssa-loop-im.c: (movement_possibility): Restrict movement of
        transaction loads.
        (tree_ssa_lim_initialize): Compute transaction bits.
        * tree.h (compute_transaction_bits): Protoize.
        * trans-mem.c (tm_region_init): Use the heap to store BB
        auxilliary data.
        (compute_transaction_bits): New.

From-SVN: r184638

gcc/ChangeLog
gcc/gimple.h
gcc/testsuite/gcc.dg/tm/pub-safety-1.c [new file with mode: 0644]
gcc/trans-mem.c
gcc/tree-ssa-loop-im.c

index c416f358abb265643c94c5bc78323b7ebe2ced9c..aae247a5a38cadef4fee1ff069fe455ae29a50ca 100644 (file)
@@ -1,3 +1,17 @@
+2012-02-28  Aldy Hernandez  <aldyh@redhat.com>
+
+       PR middle-end/51752
+       * gimple.h (gimple_in_transaction): New.
+       (gimple_set_in_transaction): New.
+       (struct gimple_statement_base): Add in_transaction field.
+       * tree-ssa-loop-im.c: (movement_possibility): Restrict movement of
+       transaction loads.
+       (tree_ssa_lim_initialize): Compute transaction bits.
+       * tree.h (compute_transaction_bits): Protoize.
+       * trans-mem.c (tm_region_init): Use the heap to store BB
+       auxilliary data.
+       (compute_transaction_bits): New.
+
 2012-02-28  Bernhard Reutner-Fischer  <aldot@gcc.gnu.org>
 
        * gcc.c (display_help): Document --help=common and sort entries
index b2b345edfabac1631d1bfaba530e0b70c425c0dd..92edd181059c178375cade8b6f7af8cc41282747 100644 (file)
@@ -305,8 +305,10 @@ struct GTY(()) gimple_statement_base {
   /* Nonzero if this statement contains volatile operands.  */
   unsigned has_volatile_ops    : 1;
 
-  /* Padding to get subcode to 16 bit alignment.  */
-  unsigned pad                 : 1;
+  /* Nonzero if this statement appears inside a transaction.  This bit
+     is calculated on de-mand and has relevant information only after
+     it has been calculated with compute_transaction_bits.  */
+  unsigned in_transaction      : 1;
 
   /* The SUBCODE field can be used for tuple-specific flags for tuples
      that do not require subcodes.  Note that SUBCODE should be at
@@ -1120,6 +1122,7 @@ extern tree omp_reduction_init (tree, tree);
 
 /* In trans-mem.c.  */
 extern void diagnose_tm_safe_errors (tree);
+extern void compute_transaction_bits (void);
 
 /* In tree-nested.c.  */
 extern void lower_nested_functions (tree);
@@ -1584,6 +1587,21 @@ gimple_set_has_volatile_ops (gimple stmt, bool volatilep)
     stmt->gsbase.has_volatile_ops = (unsigned) volatilep;
 }
 
+/* Return true if STMT is in a transaction.  */
+
+static inline bool
+gimple_in_transaction (gimple stmt)
+{
+  return stmt->gsbase.in_transaction;
+}
+
+/* Set the IN_TRANSACTION flag to TRANSACTIONP.  */
+
+static inline void
+gimple_set_in_transaction (gimple stmt, bool transactionp)
+{
+  stmt->gsbase.in_transaction = (unsigned) transactionp;
+}
 
 /* Return true if statement STMT may access memory.  */
 
diff --git a/gcc/testsuite/gcc.dg/tm/pub-safety-1.c b/gcc/testsuite/gcc.dg/tm/pub-safety-1.c
new file mode 100644 (file)
index 0000000..660e9a6
--- /dev/null
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-options "-fgnu-tm -O1 -fdump-tree-lim1" } */
+
+/* Test that thread visible loads do not get hoisted out of loops if
+   the load would not have occurred on each path out of the loop.  */
+
+int x[10] = {0,0,0,0,0,0,0,0,0,0};
+int DATA_DATA = 0;
+
+void reader()
+{
+  int i;
+  __transaction_atomic
+    { 
+      for (i = 0; i < 10; i++)
+        if (x[i])
+          x[i] += DATA_DATA;
+      /* If we loaded DATA_DATA here, we could hoist it before the loop,
+        but since we don't... we can't.  */
+    }
+}
+
+/* { dg-final { scan-tree-dump-times "Cannot hoist.*DATA_DATA because it is in a transaction" 1 "lim1" } } */
+/* { dg-final { cleanup-tree-dump "lim1" } } */
index aa330dd95c4bb8ec9a09300896a1682dff0ae52f..56ef72ea5f34081fd604d525f902f533caf0639b 100644 (file)
@@ -1858,18 +1858,25 @@ tm_region_init (struct tm_region *region)
   VEC(basic_block, heap) *queue = NULL;
   bitmap visited_blocks = BITMAP_ALLOC (NULL);
   struct tm_region *old_region;
+  struct tm_region **region_worklist;
 
   all_tm_regions = region;
   bb = single_succ (ENTRY_BLOCK_PTR);
 
+  /* We could store this information in bb->aux, but we may get called
+     through get_all_tm_blocks() from another pass that may be already
+     using bb->aux.  */
+  region_worklist =
+    (struct tm_region **) xcalloc (sizeof (struct tm_region *),
+                                 n_basic_blocks + NUM_FIXED_BLOCKS + 2);
+
   VEC_safe_push (basic_block, heap, queue, bb);
-  gcc_assert (!bb->aux);       /* FIXME: Remove me.  */
-  bb->aux = region;
+  region_worklist[bb->index] = region;
   do
     {
       bb = VEC_pop (basic_block, queue);
-      region = (struct tm_region *)bb->aux;
-      bb->aux = NULL;
+      region = region_worklist[bb->index];
+      region_worklist[bb->index] = NULL;
 
       /* Record exit and irrevocable blocks.  */
       region = tm_region_init_1 (region, bb);
@@ -1886,20 +1893,20 @@ tm_region_init (struct tm_region *region)
          {
            bitmap_set_bit (visited_blocks, e->dest->index);
            VEC_safe_push (basic_block, heap, queue, e->dest);
-           gcc_assert (!e->dest->aux); /* FIXME: Remove me.  */
 
            /* If the current block started a new region, make sure that only
               the entry block of the new region is associated with this region.
               Other successors are still part of the old region.  */
            if (old_region != region && e->dest != region->entry_block)
-             e->dest->aux = old_region;
+             region_worklist[e->dest->index] = old_region;
            else
-             e->dest->aux = region;
+             region_worklist[e->dest->index] = region;
          }
     }
   while (!VEC_empty (basic_block, queue));
   VEC_free (basic_block, heap, queue);
   BITMAP_FREE (visited_blocks);
+  free (region_worklist);
 }
 
 /* The "gate" function for all transactional memory expansion and optimization
@@ -2424,6 +2431,42 @@ get_tm_region_blocks (basic_block entry_block,
   return bbs;
 }
 
+/* Set the IN_TRANSACTION for all gimple statements that appear in a
+   transaction.  */
+
+void
+compute_transaction_bits (void)
+{
+  struct tm_region *region;
+  VEC (basic_block, heap) *queue;
+  unsigned int i;
+  gimple_stmt_iterator gsi;
+  basic_block bb;
+
+  /* ?? Perhaps we need to abstract gate_tm_init further, because we
+     certainly don't need it to calculate CDI_DOMINATOR info.  */
+  gate_tm_init ();
+
+  for (region = all_tm_regions; region; region = region->next)
+    {
+      queue = get_tm_region_blocks (region->entry_block,
+                                   region->exit_blocks,
+                                   region->irr_blocks,
+                                   NULL,
+                                   /*stop_at_irr_p=*/true);
+      for (i = 0; VEC_iterate (basic_block, queue, i, bb); ++i)
+       for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+         {
+           gimple stmt = gsi_stmt (gsi);
+           gimple_set_in_transaction (stmt, true);
+         }
+      VEC_free (basic_block, heap, queue);
+    }
+
+  if (all_tm_regions)
+    bitmap_obstack_release (&tm_obstack);
+}
+
 /* Entry point to the MARK phase of TM expansion.  Here we replace
    transactional memory statements with calls to builtins, and function
    calls with their transactional clones (if available).  But we don't
index 3c7bb65e0bf43f3f4f62803f7e57c3d8b63bbd40..ce5eb208850c60f96ed880e1430b1ff6b6212354 100644 (file)
@@ -150,7 +150,7 @@ typedef struct mem_ref
 
   bitmap indep_ref;            /* The set of memory references on that
                                   this reference is independent.  */
-  bitmap dep_ref;              /* The complement of DEP_REF.  */
+  bitmap dep_ref;              /* The complement of INDEP_REF.  */
 } *mem_ref_p;
 
 DEF_VEC_P(mem_ref_p);
@@ -412,6 +412,26 @@ movement_possibility (gimple stmt)
       || gimple_could_trap_p (stmt))
     return MOVE_PRESERVE_EXECUTION;
 
+  /* Non local loads in a transaction cannot be hoisted out.  Well,
+     unless the load happens on every path out of the loop, but we
+     don't take this into account yet.  */
+  if (flag_tm
+      && gimple_in_transaction (stmt)
+      && gimple_assign_single_p (stmt))
+    {
+      tree rhs = gimple_assign_rhs1 (stmt);
+      if (DECL_P (rhs) && is_global_var (rhs))
+       {
+         if (dump_file)
+           {
+             fprintf (dump_file, "Cannot hoist conditional load of ");
+             print_generic_expr (dump_file, rhs, TDF_SLIM);
+             fprintf (dump_file, " because it is in a transaction.\n");
+           }
+         return MOVE_IMPOSSIBLE;
+       }
+    }
+
   return ret;
 }
 
@@ -2387,6 +2407,9 @@ tree_ssa_lim_initialize (void)
   sbitmap_free (contains_call);
 
   lim_aux_data_map = pointer_map_create ();
+
+  if (flag_tm)
+    compute_transaction_bits ();
 }
 
 /* Cleans up after the invariant motion pass.  */