Re: [PATCH] Fix PR70484, RTL DSE using wrong dependence check

[Richard Biener <rguenther at suse dot de>]

On Mon, 4 Apr 2016, Richard Biener wrote:

> On Fri, 1 Apr 2016, Bernd Schmidt wrote:
> 
> > On 04/01/2016 11:08 AM, Richard Biener wrote:
> > >    	{
> > > ! 	  if (canon_true_dependence (s_info->mem,
> > > ! 				     GET_MODE (s_info->mem),
> > > ! 				     s_info->mem_addr,
> > > ! 				     mem, mem_addr))
> > >    	    {
> > >    	      s_info->rhs = NULL;
> > >    	      s_info->const_rhs = NULL;
> > > --- 1609,1617 ----
> > >    	   the value of store_info.  If it is, set the rhs to NULL to
> > >    	   keep it from being used to remove a load.  */
> > >    	{
> > > ! 	  if (canon_output_dependence (s_info->mem, true,
> > > ! 				       mem, GET_MODE (mem),
> > > ! 				       mem_addr))
> > >    	    {
> > >    	      s_info->rhs = NULL;
> > >    	      s_info->const_rhs = NULL;
> > 
> > I think the patch is ok, but there is a comment in that function which
> > references canon_true_dependence; that should also be fixed up.
> 
> Done, though I don't understand it at all ... if alias-set was supposed
> to be zero for all cases we call canon_true_dependence then the issue
> wouldn't have happened.  Maybe there was times where passing mem_addr
> == NULL_RTX to canon_true_dependence caused it to bail out?
> 
> Not sure how to adjust that comment now, maybe it would be valid
> to simply remove the if (spill_alias_set) case and always use
> the else case?
> 
> > Isn't the testcase invalid though? I thought accesses through char * 
> > pointers bypass aliasing rules, but accessing a char array through int * 
> > and long * pointers doesn't?
> 
> I have installed it as the following with adjusted testcase, if somebody 
> can shed some light on the odd comment I'll happily followup.

Jakub added that comment in r146669.  Still the code only handles
alias-sets being different specially by not deleting such stores.
But it doesn't break from the loop there (if that store is aliasing).

I suppose this code might have similar issues for deleting "dead"
stores when not replacing the RHS.

Richard.

> Richard.
> 
> 2016-04-04  Richard Biener  <rguenther@suse.de>
> 
> 	PR rtl-optimization/70484
> 	* rtl.h (canon_output_dependence): Declare.
> 	* alias.c (canon_output_dependence): New function.
> 	* dse.c (record_store): Use canon_output_dependence rather
> 	than canon_true_dependence.
> 
> 	* gcc.dg/torture/pr70484.c: New testcase.
> 
> Index: gcc/rtl.h
> ===================================================================
> *** gcc/rtl.h	(revision 234663)
> --- gcc/rtl.h	(working copy)
> *************** extern int anti_dependence (const_rtx, c
> *** 3652,3657 ****
> --- 3652,3659 ----
>   extern int canon_anti_dependence (const_rtx, bool,
>   				  const_rtx, machine_mode, rtx);
>   extern int output_dependence (const_rtx, const_rtx);
> + extern int canon_output_dependence (const_rtx, bool,
> + 				    const_rtx, machine_mode, rtx);
>   extern int may_alias_p (const_rtx, const_rtx);
>   extern void init_alias_target (void);
>   extern void init_alias_analysis (void);
> Index: gcc/alias.c
> ===================================================================
> *** gcc/alias.c	(revision 234663)
> --- gcc/alias.c	(working copy)
> *************** output_dependence (const_rtx mem, const_
> *** 3057,3062 ****
> --- 3057,3076 ----
>   			     /*mem_canonicalized=*/false,
>   			     /*x_canonicalized*/false, /*writep=*/true);
>   }
> + 
> + /* Likewise, but we already have a canonicalized MEM, and X_ADDR for X.
> +    Also, consider X in X_MODE (which might be from an enclosing
> +    STRICT_LOW_PART / ZERO_EXTRACT).
> +    If MEM_CANONICALIZED is true, MEM is canonicalized.  */
> + 
> + int
> + canon_output_dependence (const_rtx mem, bool mem_canonicalized,
> + 			 const_rtx x, machine_mode x_mode, rtx x_addr)
> + {
> +   return write_dependence_p (mem, x, x_mode, x_addr,
> + 			     mem_canonicalized, /*x_canonicalized=*/true,
> + 			     /*writep=*/true);
> + }
>   
>   
>   
> Index: gcc/dse.c
> ===================================================================
> *** gcc/dse.c	(revision 234663)
> --- gcc/dse.c	(working copy)
> *************** record_store (rtx body, bb_info_t bb_inf
> *** 1609,1618 ****
>   	   the value of store_info.  If it is, set the rhs to NULL to
>   	   keep it from being used to remove a load.  */
>   	{
> ! 	  if (canon_true_dependence (s_info->mem,
> ! 				     GET_MODE (s_info->mem),
> ! 				     s_info->mem_addr,
> ! 				     mem, mem_addr))
>   	    {
>   	      s_info->rhs = NULL;
>   	      s_info->const_rhs = NULL;
> --- 1609,1617 ----
>   	   the value of store_info.  If it is, set the rhs to NULL to
>   	   keep it from being used to remove a load.  */
>   	{
> ! 	  if (canon_output_dependence (s_info->mem, true,
> ! 				       mem, GET_MODE (mem),
> ! 				       mem_addr))
>   	    {
>   	      s_info->rhs = NULL;
>   	      s_info->const_rhs = NULL;
> Index: gcc/testsuite/gcc.dg/torture/pr70484.c
> ===================================================================
> *** gcc/testsuite/gcc.dg/torture/pr70484.c	(revision 0)
> --- gcc/testsuite/gcc.dg/torture/pr70484.c	(revision 0)
> ***************
> *** 0 ****
> --- 1,19 ----
> + /* { dg-do run } */
> + 
> + extern void abort (void);
> + 
> + int __attribute__((noinline,noclone))
> + f(int *pi, long *pl)
> + {
> +   *pi = 1;
> +   *pl = 0;
> +   return *(char *)pi;
> + }
> + 
> + int main()
> + {
> +   union { long l; int i; } a;
> +   if (f (&a.i, &a.l) != 0)
> +     abort ();
> +   return 0;
> + }
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)