Patchwork [trans-mem] fix transaction_callable's with asm statements

login
register
mail settings
Submitter Aldy Hernandez
Date Jan. 11, 2011, 7:30 p.m.
Message ID <4D2CAFEB.2020607@redhat.com>
Download mbox | patch
Permalink /patch/78424/
State New
Headers show

Comments

Aldy Hernandez - Jan. 11, 2011, 7:30 p.m.
One day when I'm bored, I'm going to rewrite this IPA TM pass.  It's an 
ever present source of grief in my life.  But alas, not today...

Today's pain begins here:

        __attribute__((transaction_callable))
        void func()
        {
           __asm__ ("");
        }

We scan the function in diagnose_tm() and set the tm_may_enter_irr bit 
in the cgraph node.  However, in the TM IPA pass, we don't mark 
irrevocable basic blocks (in ipa_tm_scan_irr_function()) unless the 
function is already in the (irrevocable) worklist.

This patch adds functions marked with tm_may_enter_irr into the 
worklist, so we can later pick them up and scan their basic blocks for 
irrevocability, thus going irrevocable at the correct place.

There is also a gratuitous fix to instrumentation of constructors.  We 
currently ICE on aggregate types.

All this is actually background for an upcoming one liner which has 
uncovered all sorts of pain throughout the TM engine.

OK for branch?
* trans-mem.c (ipa_tm_execute): Place possibly irrevocable
	functions in worklist.
	(build_tm_store): Only call build_int_cst with integers.
Richard Henderson - Jan. 12, 2011, 6:10 p.m.
>        /* Handle the easy initialization to zero.  */
> -      if (CONSTRUCTOR_ELTS (rhs) == 0)
> +      if (CONSTRUCTOR_ELTS (rhs) == 0 && INTEGRAL_TYPE_P (type))
>  	rhs = build_int_cst (type, 0);

While this works, it's not what I intended.  In particular,
you'll never see a CONSTRUCTOR for an integral type, so this
condition will never fire.

What's intended here is to store a zero of the proper size
into the location.  So actually the little change is

  rhs = build_int_cst (simple_type, 0);

This should probably be done as a separate fix, since it
turns out to be non-obvious.

> +      /* If we saw something that will make us go irrevocable, put it
> +	 in the worklist so we can scan the function later
> +	 (ipa_tm_scan_irr_function) and mark the irrevocable
> +	 blocks.  */
> +      if (node->local.tm_may_enter_irr)
> +	maybe_push_queue (node, &worklist, &d->in_worklist);

Ok.  You're probably right that the ipa pass needs to be rewritten.
Third time's the charm, right?


r~

Patch

Index: testsuite/c-c++-common/tm/inline-asm-2.c
===================================================================
--- testsuite/c-c++-common/tm/inline-asm-2.c	(revision 0)
+++ testsuite/c-c++-common/tm/inline-asm-2.c	(revision 0)
@@ -0,0 +1,8 @@ 
+// { dg-do compile }
+// { dg-options "-fgnu-tm" }
+
+__attribute__((transaction_callable))
+void func()
+{
+  __asm__ ("");
+}
Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 168675)
+++ trans-mem.c	(working copy)
@@ -2067,7 +2067,7 @@  build_tm_store (location_t loc, tree lhs
   if (TREE_CODE (rhs) == CONSTRUCTOR)
     {
       /* Handle the easy initialization to zero.  */
-      if (CONSTRUCTOR_ELTS (rhs) == 0)
+      if (CONSTRUCTOR_ELTS (rhs) == 0 && INTEGRAL_TYPE_P (type))
 	rhs = build_int_cst (type, 0);
       else
 	{
@@ -4498,6 +4498,13 @@  ipa_tm_execute (void)
       a = cgraph_function_body_availability (node);
       d = get_cg_data (node);
 
+      /* If we saw something that will make us go irrevocable, put it
+	 in the worklist so we can scan the function later
+	 (ipa_tm_scan_irr_function) and mark the irrevocable
+	 blocks.  */
+      if (node->local.tm_may_enter_irr)
+	maybe_push_queue (node, &worklist, &d->in_worklist);
+
       /* Some callees cannot be arbitrarily cloned.  These will always be
 	 irrevocable.  Mark these now, so that we need not scan them.  */
       if (is_tm_irrevocable (node->decl))