[PATCH] Fix PR84003

[Richard Biener]

On Fri, 26 Jan 2018, Richard Biener wrote:

> On Fri, 26 Jan 2018, Jakub Jelinek wrote:
> 
> > On Thu, Jan 25, 2018 at 12:18:21PM +0100, Richard Biener wrote:
> > > 2018-01-25  Richard Biener  <rguenther@suse.de>
> > > 
> > > 	PR rtl-optimization/84003
> > > 	* dse.c (dse_step1): When removing redundant stores make sure
> > > 	to adjust the earlier stores alias-set to match semantics of
> > > 	the removed one and its own.
> > > 	(dse_step6): Likewise.
> > > 
> > > 	* g++.dg/torture/pr77745.C: Mark foo noinline to trigger
> > > 	latent bug in DSE.
> > > 
> > > Index: gcc/dse.c
> > > ===================================================================
> > > --- gcc/dse.c	(revision 257043)
> > > +++ gcc/dse.c	(working copy)
> > > @@ -2725,6 +2725,19 @@ dse_step1 (void)
> > >  					    "eliminated\n",
> > >  				 INSN_UID (ptr->insn),
> > >  				 INSN_UID (s_info->redundant_reason->insn));
> > > +		      /* If the later store we delete could have changed the
> > > +		         dynamic type of the memory make sure the one we
> > > +			 preserve serves as a store for all loads that could
> > > +			 validly have accessed the one we delete.  */
> > > +		      store_info *r_info = s_info->redundant_reason->store_rec;
> > > +		      while (r_info)
> > > +			{
> > > +			  if (r_info->is_set
> > > +			      && (MEM_ALIAS_SET (s_info->mem)
> > > +				  != MEM_ALIAS_SET (r_info->mem)))
> > > +			    set_mem_alias_set (r_info->mem, 0);
> > 
> > Is alias set 0 the only easily computable choice if there is a mismatch?
> > Also, isn't it fine if one of the alias set is a subset of the other one?
> > 
> > More importantly, I think set_mem_alias_set (r_info->mem, 0) can't work,
> > r_info->mem is a result of canon_rtx (SET_DEST (body)), so sometimes could
> > be the MEM that appears in the instruction, but at other times could be a
> > different pointer and changing that wouldn't change anything in the actual
> > instruction.  So, in order to do this you'd need to add probably another
> > field and record the original SET_DEST (body) record_store is called with.
> 
> Uh, indeed.  See my mail in response to Richard which comes up with
> an alternate patch avoiding this issue.
> 
> > Even that doesn't need to be something that really appears in the
> > instruction, but the exception in that case is the memset call and that
> > semantically has alias set of 0.
> >
> > > --- gcc/testsuite/g++.dg/torture/pr77745.C	(revision 257043)
> > > +++ gcc/testsuite/g++.dg/torture/pr77745.C	(working copy)
> > > @@ -2,7 +2,7 @@
> > >  
> > >  inline void* operator new(__SIZE_TYPE__, void* __p) noexcept { return __p; }
> > >  
> > > -long foo(char *c1, char *c2)
> > > +long __attribute__((noinline)) foo(char *c1, char *c2)
> > >  {
> > >    long *p1 = new (c1) long;
> > >    *p1 = 100;
> > 
> > Is it desirable to modify an existing test, rather than say macroize this
> > and add pr77745-2.C that will define some macro and include this pr77745.C,
> > such that we cover both noinline and without noinline?
> 
> Yeah, I'll do that.

This is what I have applied, bootstrapped and tested on 
x86_64-unknown-linux-gnu.

Richard.

2018-01-26  Richard Biener  <rguenther@suse.de>

	PR rtl-optimization/84003
	* dse.c (record_store): Only record redundant stores when
	the earlier store aliases at least all accesses the later one does.

	* g++.dg/torture/pr77745.C: Mark foo noinline to trigger
	latent bug in DSE if NOINLINE is appropriately defined.
	* g++.dg/torture/pr77745-2.C: New testcase including pr77745.C
	and defining NOINLINE.

Index: gcc/dse.c
===================================================================
--- gcc/dse.c	(revision 257077)
+++ gcc/dse.c	(working copy)
@@ -1532,7 +1532,12 @@ record_store (rtx body, bb_info_t bb_inf
 	      && known_subrange_p (offset, width,
 				   s_info->offset, s_info->width)
 	      && all_positions_needed_p (s_info, offset - s_info->offset,
-					 width))
+					 width)
+	      /* We can only remove the later store if the earlier aliases
+		 at least all accesses the later one.  */
+	      && (MEM_ALIAS_SET (mem) == MEM_ALIAS_SET (s_info->mem)
+		  || alias_set_subset_of (MEM_ALIAS_SET (mem),
+					  MEM_ALIAS_SET (s_info->mem))))
 	    {
 	      if (GET_MODE (mem) == BLKmode)
 		{
Index: gcc/testsuite/g++.dg/torture/pr77745.C
===================================================================
--- gcc/testsuite/g++.dg/torture/pr77745.C	(revision 257080)
+++ gcc/testsuite/g++.dg/torture/pr77745.C	(working copy)
@@ -1,8 +1,12 @@
 // { dg-do run }
 
+#ifndef NOINLINE
+#define NOINLINE /* */
+#endif
+
 inline void* operator new(__SIZE_TYPE__, void* __p) noexcept { return __p; }
 
-long foo(char *c1, char *c2)
+long NOINLINE foo(char *c1, char *c2)
 {
   long *p1 = new (c1) long;
   *p1 = 100;
Index: gcc/testsuite/g++.dg/torture/pr77745-2.C
===================================================================
--- gcc/testsuite/g++.dg/torture/pr77745-2.C	(nonexistent)
+++ gcc/testsuite/g++.dg/torture/pr77745-2.C	(working copy)
@@ -0,0 +1,4 @@
+// { dg-do run }
+
+#define NOINLINE __attribute__((noinline))
+#include "pr77745.C"