diff mbox

Fix PR51125 (was: death@scope broke TM tests)

Message ID Pine.LNX.4.64.1111211625070.26507@wotan.suse.de
State New
Headers show

Commit Message

Michael Matz Nov. 21, 2011, 3:42 p.m. UTC 
Hi,

On Fri, 18 Nov 2011, Aldy Hernandez wrote:

> I just CC'ed you on a bug that I believe was caused by your patch.
> 
> I forgot to CC you on the bug before I wrote the comment on it.
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51125#c4
> 
> Perhaps you can take a look?

The attached patch fixes the problem.  The TM machinery tried to deal with 
the death marker as if it was a real assignment leading to the trouble.  
The patch avoids this (leaving the clobber for expand to see).

As I'm not totally sure about semantics of TM I don't know if the 
occurrence of this problem doesn't also point to a missed optimization for 
the TM handling.  The situation after gimplification looks basically like 
so:

        __transaction_atomic
          {
            {
              struct shared_count sc;
              try
                // ctor(&sc), dtor(&sc)
              finally
                {
                  sc = {CLOBBER};
                }
            }
          }

later lowered to:

<bb 3>:
  __transaction_atomic  // SUBCODE=[ GTMA_HAVE_STORE ]
<bb 4>:
  // ctor(&sc),dtor(&sc) [tm-clone]
  goto <bb 7>;

<bb 7>:
  sc ={v} {CLOBBER};
  __builtin__ITM_commitTransaction ();

<bb 8>:
  return 0;

<L4>:
  landing-pad

The object 'sc' is constructed on the stack.  Is it necessary to make it 
transact?  If not, then you can possibly improve code quality somewhat by 
ignoring the clobbers also in requires_barrier (which then would need 
changes to receive the whole statement, not just the lhs/rhs to determine 
if this is an ignorable statement).  I haven't tried this, though.  The 
patch below would be necessary anyway.

In any case, patch fixes the testcases again, regstrapping on x86_64-linux 
in progress.  Okay for trunk?


Ciao,
Michael.

	PR other/51125
	* trans-mem.c (expand_block_tm): Ignore clobbers.

Comments

Richard Henderson Nov. 21, 2011, 11:39 p.m. UTC | #1 
On 11/21/2011 07:42 AM, Michael Matz wrote:
>         __transaction_atomic
>           {
>             {
>               struct shared_count sc;
>               try
>                 // ctor(&sc), dtor(&sc)
>               finally
>                 {
>                   sc = {CLOBBER};
>                 }
>             }
>           }
> 
> later lowered to:
> 
> <bb 3>:
>   __transaction_atomic  // SUBCODE=[ GTMA_HAVE_STORE ]
> <bb 4>:
>   // ctor(&sc),dtor(&sc) [tm-clone]
>   goto <bb 7>;
> 
> <bb 7>:
>   sc ={v} {CLOBBER};
>   __builtin__ITM_commitTransaction ();
> 
> <bb 8>:
>   return 0;
> 
> <L4>:
>   landing-pad
> 
> The object 'sc' is constructed on the stack.  Is it necessary to make it 
> transact?  If not, then you can possibly improve code quality somewhat by 
> ignoring the clobbers also in requires_barrier (which then would need 
> changes to receive the whole statement, not just the lhs/rhs to determine 
> if this is an ignorable statement).  I haven't tried this, though.  The 
> patch below would be necessary anyway.

No, SC need not transact because it is already transaction-local.

> 	PR other/51125
> 	* trans-mem.c (expand_block_tm): Ignore clobbers.

Ok.


r~
diff mbox

Patch

Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 181582)
+++ trans-mem.c	(working copy)
@@ -2319,7 +2319,8 @@  expand_block_tm (struct tm_region *regio
 	{
 	case GIMPLE_ASSIGN:
 	  /* Only memory reads/writes need to be instrumented.  */
-	  if (gimple_assign_single_p (stmt))
+	  if (gimple_assign_single_p (stmt)
+	      && !gimple_clobber_p (stmt))
 	    {
 	      expand_assign_tm (region, &gsi);
 	      continue;