This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.

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;
+ }


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]