[PATCH] Fix PR84003

Jakub Jelinek jakub@redhat.com
Fri Jan 26 11:15:00 GMT 2018


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.
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?

	Jakub



More information about the Gcc-patches mailing list