diff mbox

trans-mem: virtual ops for gimple_transaction

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

Commit Message

Richard Biener Feb. 10, 2012, 9:44 a.m. UTC 
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 ...

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.

Comments

Richard Henderson Feb. 10, 2012, 5:53 p.m. UTC | #1 
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~
Richard Biener Feb. 13, 2012, 9:35 a.m. UTC | #2 
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.
Richard Henderson Feb. 13, 2012, 5:08 p.m. UTC | #3 
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~
Richard Biener Feb. 14, 2012, 8:38 a.m. UTC | #4 
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.
diff mbox

Patch

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