trans-mem: virtual ops for gimple_transaction

trans-mem: virtual ops for gimple_transaction

[Richard Henderson]

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

> From: rguenth at gcc dot gnu.org <gcc-bugzilla@gcc.gnu.org>
> the __transaction_atomic  // SUBCODE=[ GTMA_HAVE_STORE ] statement
> looks like an overly optimistic way to start a transaction in my quick view.

Indeed.  At some point this worked, but this may have gotten lost
during one of the merges.  Now,

  # .MEM_8 = VDEF <.MEM_7(D)>
  __transaction_relaxed  // SUBCODE=[ ... ]

Bootstrapped and tested on x86_64.  Ok?


r~

[-- Attachment #2: z --]
[-- Type: text/plain, Size: 2671 bytes --]

	* tree-ssa-dce.c (propagate_necessity): Handle GIMPLE_TRANSACTION.
	* tree-ssa-operands.c (parse_ssa_operands): Add virtual operands
	for GIMPLE_TRANSACTION.  Tidy if's into a switch.


diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index ccdf14a..ace9ef9 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -965,6 +965,13 @@ propagate_necessity (struct edge_list *el)
 		    mark_aliased_reaching_defs_necessary (stmt, op);
 		}
 	    }
+	  else if (gimple_code (stmt) == GIMPLE_TRANSACTION)
+	    {
+	      /* The beginning of a transaction is a memory barrier.  */
+	      /* ??? If we were really cool, we'd only be a barrier
+		 for the memories touched within the transaction.  */
+	      mark_all_reaching_defs_necessary (stmt);
+	    }
 	  else
 	    gcc_unreachable ();
 
diff --git a/gcc/tree-ssa-operands.c b/gcc/tree-ssa-operands.c
index 0045dd8..ed0d34d 100644
--- a/gcc/tree-ssa-operands.c
+++ b/gcc/tree-ssa-operands.c
@@ -1043,35 +1043,46 @@ static void
 parse_ssa_operands (gimple stmt)
 {
   enum gimple_code code = gimple_code (stmt);
+  size_t i, n, start = 0;
 
-  if (code == GIMPLE_ASM)
-    get_asm_expr_operands (stmt);
-  else if (is_gimple_debug (stmt))
+  switch (code)
     {
+    case GIMPLE_ASM:
+      get_asm_expr_operands (stmt);
+      break;
+
+    case GIMPLE_TRANSACTION:
+      /* The start of a transaction is a memory barrier.  */
+      add_virtual_operand (stmt, opf_def | opf_use);
+      break;
+
+    case GIMPLE_DEBUG:
       if (gimple_debug_bind_p (stmt)
 	  && gimple_debug_bind_has_value_p (stmt))
 	get_expr_operands (stmt, gimple_debug_bind_get_value_ptr (stmt),
 			   opf_use | opf_no_vops);
-    }
-  else
-    {
-      size_t i, start = 0;
+      break;
 
-      if (code == GIMPLE_ASSIGN || code == GIMPLE_CALL)
-	{
-	  get_expr_operands (stmt, gimple_op_ptr (stmt, 0), opf_def);
-	  start = 1;
-	}
-
-      for (i = start; i < gimple_num_ops (stmt); i++)
-	get_expr_operands (stmt, gimple_op_ptr (stmt, i), opf_use);
+    case GIMPLE_RETURN:
+      append_vuse (gimple_vop (cfun));
+      goto do_default;
 
+    case GIMPLE_CALL:
       /* Add call-clobbered operands, if needed.  */
-      if (code == GIMPLE_CALL)
-	maybe_add_call_vops (stmt);
+      maybe_add_call_vops (stmt);
+      /* FALLTHRU */
 
-      if (code == GIMPLE_RETURN)
-	append_vuse (gimple_vop (cfun));
+    case GIMPLE_ASSIGN:
+      get_expr_operands (stmt, gimple_op_ptr (stmt, 0), opf_def);
+      start = 1;
+      /* FALLTHRU */
+
+    default:
+    do_default:
+      n = gimple_num_ops (stmt);
+      for (i = start; i < n; i++)
+	get_expr_operands (stmt, gimple_op_ptr (stmt, i), opf_use);
+      break;
     }
 }
 

Re: trans-mem: virtual ops for gimple_transaction

[Torvald Riegel]

On Thu, 2012-02-09 at 15:05 -0800, Richard Henderson wrote:
> +             /* The beginning of a transaction is a memory barrier.
> */
> +             /* ??? If we were really cool, we'd only be a barrier
> +                for the memories touched within the transaction.  */

Why?  I'm not quite sure what kind of memory barrier you mean here, but
a transaction can synchronize with other transactions and thus should be
a barrier for all memory accesses, or not?  We could move safe code into
it, but not out of it or across it.  We could perhaps move
transaction_pure code across it, or out of it, IIRC.


Torvald

Re: trans-mem: virtual ops for gimple_transaction

[Richard Guenther]

On Thu, 9 Feb 2012, Richard Henderson wrote:

> > From: rguenth at gcc dot gnu.org <gcc-bugzilla@gcc.gnu.org>
> > the __transaction_atomic  // SUBCODE=[ GTMA_HAVE_STORE ] statement
> > looks like an overly optimistic way to start a transaction in my quick view.
> 
> Indeed.  At some point this worked, but this may have gotten lost
> during one of the merges.  Now,
> 
>   # .MEM_8 = VDEF <.MEM_7(D)>
>   __transaction_relaxed  // SUBCODE=[ ... ]
> 
> Bootstrapped and tested on x86_64.  Ok?

Yes.

But ...

diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
index ccdf14a..ace9ef9 100644
--- a/gcc/tree-ssa-dce.c
+++ b/gcc/tree-ssa-dce.c
@@ -965,6 +965,13 @@ propagate_necessity (struct edge_list *el)
                    mark_aliased_reaching_defs_necessary (stmt, op);
                }
            }
+         else if (gimple_code (stmt) == GIMPLE_TRANSACTION)
+           {
+             /* The beginning of a transaction is a memory barrier.  */
+             /* ??? If we were really cool, we'd only be a barrier
+                for the memories touched within the transaction.  */
+             mark_all_reaching_defs_necessary (stmt);
+           }
          else
            gcc_unreachable ();

hints at that code might not expect virtual operands on this ...

What is the reason to keep a GIMPLE_TRANSACTION stmt after
TM lowering and not lower it to a builtin function call?  It seems
the body is empty after lowering (what's the label thing?)
I'd have arranged it do lower

  __transaction_atomic
    {
      {
        x[9] = 1;
      }
    }

to

  __builtin_transaction_atomic (GTMA_HAVE_STORE, &label);
  try
    {
      x[9] = 1;
    }
  finally
    {
      __builtin__ITM_commitTransaction ();
    }

Eventually returning/passing a token to link a transaction start
to its commit / abort.

That said, I expect some more fallout from the patch, but I suppose
TM is still experimental enough that we can fixup things later.

Richard.

Re: trans-mem: virtual ops for gimple_transaction

[Richard Henderson]

On 02/10/2012 01:44 AM, Richard Guenther wrote:
> What is the reason to keep a GIMPLE_TRANSACTION stmt after
> TM lowering and not lower it to a builtin function call?

Because "real" optimization hasn't happened yet, and we hold
out hope that we'll be able to delete stuff as unreachable.
Especially all instances of transaction_cancel.

> It seems the body is empty after lowering (what's the label thing?)

The label is the transaction cancel label.

When we finally convert GIMPLE_TRANSACTION a builtin, we'll
generate different code layouts with and without a cancel.


r~

Re: trans-mem: virtual ops for gimple_transaction

[Richard Guenther]

On Fri, 10 Feb 2012, Richard Henderson wrote:

> On 02/10/2012 01:44 AM, Richard Guenther wrote:
> > What is the reason to keep a GIMPLE_TRANSACTION stmt after
> > TM lowering and not lower it to a builtin function call?
> 
> Because "real" optimization hasn't happened yet, and we hold
> out hope that we'll be able to delete stuff as unreachable.
> Especially all instances of transaction_cancel.
> 
> > It seems the body is empty after lowering (what's the label thing?)
> 
> The label is the transaction cancel label.
> 
> When we finally convert GIMPLE_TRANSACTION a builtin, we'll
> generate different code layouts with and without a cancel.

Ah, I see.  But wouldn't a placeholder builtin function be
effectively the same as using a new GIMPLE stmt kind?

Richard.

Re: trans-mem: virtual ops for gimple_transaction

[Richard Henderson]

On 02/13/2012 01:35 AM, Richard Guenther wrote:
> On Fri, 10 Feb 2012, Richard Henderson wrote:
> 
>> On 02/10/2012 01:44 AM, Richard Guenther wrote:
>>> What is the reason to keep a GIMPLE_TRANSACTION stmt after
>>> TM lowering and not lower it to a builtin function call?
>>
>> Because "real" optimization hasn't happened yet, and we hold
>> out hope that we'll be able to delete stuff as unreachable.
>> Especially all instances of transaction_cancel.
>>
>>> It seems the body is empty after lowering (what's the label thing?)
>>
>> The label is the transaction cancel label.
>>
>> When we finally convert GIMPLE_TRANSACTION a builtin, we'll
>> generate different code layouts with and without a cancel.
> 
> Ah, I see.  But wouldn't a placeholder builtin function be
> effectively the same as using a new GIMPLE stmt kind?

Except for the whole "need to hold on to a label" thing.

Honestly, think about that for 10 seconds and tell me that
a builtin is better than simply re-tasking the gimple code
that we already have around.


r~

Re: trans-mem: virtual ops for gimple_transaction

[Richard Guenther]

On Mon, 13 Feb 2012, Richard Henderson wrote:

> On 02/13/2012 01:35 AM, Richard Guenther wrote:
> > On Fri, 10 Feb 2012, Richard Henderson wrote:
> > 
> >> On 02/10/2012 01:44 AM, Richard Guenther wrote:
> >>> What is the reason to keep a GIMPLE_TRANSACTION stmt after
> >>> TM lowering and not lower it to a builtin function call?
> >>
> >> Because "real" optimization hasn't happened yet, and we hold
> >> out hope that we'll be able to delete stuff as unreachable.
> >> Especially all instances of transaction_cancel.
> >>
> >>> It seems the body is empty after lowering (what's the label thing?)
> >>
> >> The label is the transaction cancel label.
> >>
> >> When we finally convert GIMPLE_TRANSACTION a builtin, we'll
> >> generate different code layouts with and without a cancel.
> > 
> > Ah, I see.  But wouldn't a placeholder builtin function be
> > effectively the same as using a new GIMPLE stmt kind?
> 
> Except for the whole "need to hold on to a label" thing.
> 
> Honestly, think about that for 10 seconds and tell me that
> a builtin is better than simply re-tasking the gimple code
> that we already have around.

I'm only worried about passes that would need to explicitely
care about new gimple codes (like the DCE case you spotted).
All passes have to handle calls already - and apart from the
label thingie a call would work (well, you could pass the
label by reference, but that would probably make it a possible
target for a computed goto ...).

So, well ... I guess a new gimple code is ok (we don't want
to change that now), but in general trying a little harder
to avoid new kinds of gimple in the optimizer IL is always good.

Richard.