Re: [PATCH] Handle non-volatile GIMPLE_ASM in get_references_in_stmt (PR tree-optimization/51987)

[Richard Guenther <rguenther at suse dot de>]

On Wed, 25 Jan 2012, Jakub Jelinek wrote:

> Hi!
> 
> The s390 glibc (_itoa function in particular) is miscompiled because
> pcom pass changes:
>   __asm__("lr %N0,%1
>         mr %0,%2" : "=&r" __x.__ll : "r" ti_10, "r" s1_12);
>   __w1_15 = __x.__i.__h;
> into:
>   __x___i___h_lsm0.28_34 = __x.__i.__h;
> ...
>   __asm__("lr %N0,%1
>         mr %0,%2" : "=&r" __x.__ll : "r" ti_10, "r" s1_12);
>   __w1_15 = __x___i___h_lsm0.28_34;
> (where __x is a non-addressable union var of a DImode __ll and a struct of
> two SImode __i.__{l,h}.  The problem is in get_references_in_stmt:
>   /* ASM_EXPR and CALL_EXPR may embed arbitrary side effects.
>      Calls have side-effects, except those to const or pure
>      functions.  */
>   if ((stmt_code == GIMPLE_CALL
>        && !(gimple_call_flags (stmt) & (ECF_CONST | ECF_PURE)))
>       || (stmt_code == GIMPLE_ASM
>           && gimple_asm_volatile_p (stmt)))
>     clobbers_memory = true;
> is the only place where it handles asm (but the testcase doesn't have
> it volatile) and otherwise ignores all the references.
> 
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux,
> ok for trunk?

I wonder if it's worth handling asms in any fancy way here, considering
that data-ref analysis happily punts on them completely.  Thus, why
not change the above to

  || stmt_code == GIMPLE_ASM)

?  That sounds more safe for this stage anyway (does a volatile
asm really clobber memory even if you don't explicitely say so?
At least the operand scanner won't add virtual operands just because of 
that, so the check looks bogus anyway).

> BTW, not sure about non-volatile asm that has memory clobber, maybe
> the above snippet should be changed into:
>       || (stmt_code == GIMPLE_ASM
>           && (gimple_asm_volatile_p (stmt)
> 	      || gimple_asm_clobbers_memory_p (stmt)))

Yeah, and drop the gimple_asm_volatile_p check.

Btw, there are numerous callers of get_references_in_stmt that
do not check its return value ... (and I think it misses a
return value kind that tells the vector of references may be incomplete,
like in the ASM kind right now).

Btw, get_references_in_stmt should probably use walk_stmt_load_store_ops,
so we have a single place where we put knowledge where memory references
can occur.

Thanks,
Richard.

> 2012-01-25  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/51987
> 	* tree-data-ref.c (get_references_in_stmt): Handle references in
> 	non-volatile GIMPLE_ASM.
> 
> 	* gcc.target/i386/pr51987.c: New test.
> 
> --- gcc/tree-data-ref.c.jj	2011-11-28 17:58:04.000000000 +0100
> +++ gcc/tree-data-ref.c	2012-01-24 22:33:18.995077693 +0100
> @@ -1,5 +1,5 @@
>  /* Data references and dependences detectors.
> -   Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011
> +   Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012
>     Free Software Foundation, Inc.
>     Contributed by Sebastian Pop <pop@cri.ensmp.fr>
>  
> @@ -4226,6 +4226,35 @@ get_references_in_stmt (gimple stmt, VEC
>  	    }
>  	}
>      }
> +  else if (stmt_code == GIMPLE_ASM)
> +    {
> +      unsigned i;
> +      /* Inputs may perform loads.  */
> +      for (i = 0; i < gimple_asm_ninputs (stmt); ++i)
> +	{
> +	  op1 = &TREE_VALUE (gimple_asm_input_op (stmt, i));
> +	  if (DECL_P (*op1)
> +	      || (REFERENCE_CLASS_P (*op1) && get_base_address (*op1)))
> +	    {
> +	      ref = VEC_safe_push (data_ref_loc, heap, *references, NULL);
> +	      ref->pos = op1;
> +	      ref->is_read = true;
> +	    }
> +	}
> +      /* Outputs may perform stores.  */
> +      for (i = 0; i < gimple_asm_noutputs (stmt); ++i)
> +	{
> +	  op0 = &TREE_VALUE (gimple_asm_output_op (stmt, i));
> +	  if (DECL_P (*op0)
> +	      || (REFERENCE_CLASS_P (*op0) && get_base_address (*op0)))
> +	    {
> +	      ref = VEC_safe_push (data_ref_loc, heap, *references, NULL);
> +	      ref->pos = op0;
> +	      ref->is_read = false;
> +	    }
> +	}
> +      return clobbers_memory;
> +    }
>    else
>      return clobbers_memory;
>  
> --- gcc/testsuite/gcc.target/i386/pr51987.c.jj	2012-01-24 23:32:44.887896707 +0100
> +++ gcc/testsuite/gcc.target/i386/pr51987.c	2012-01-24 23:34:26.289296794 +0100
> @@ -0,0 +1,33 @@
> +/* PR tree-optimization/51987 */
> +/* { dg-do run { target { ! { ia32 } } } } */
> +/* { dg-options "-O3" } */
> +
> +extern void abort (void);
> +union U { unsigned long long l; struct { unsigned int l, h; } i; };
> +
> +__attribute__((noinline, noclone)) void
> +foo (char *x, char *y)
> +{
> +  int i;
> +  for (i = 0; i < 64; i++)
> +    {
> +      union U u;
> +      asm ("movl %1, %k0; salq $32, %0" : "=r" (u.l) : "r" (i));
> +      x[i] = u.i.h;
> +      union U v;
> +      asm ("movl %1, %k0; salq $32, %0" : "=r" (v.l) : "r" (i));
> +      y[i] = v.i.h;
> +    }
> +}
> +
> +int
> +main ()
> +{
> +  char a[64], b[64];
> +  int i;
> +  foo (a, b);
> +  for (i = 0; i < 64; i++)
> +    if (a[i] != i || b[i] != i)
> +      abort ();
> +  return 0;
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer