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

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

[Richard Biener]

RTL DSE uses true_dependence to see whether a store may be killed by
anothe store - that's obviously broken.  The following patch makes
it use output_dependence instead (introducing a canon_ variant of that).

Bootstrap & regtest running on x86_64-unknown-linux-gnu.

Ok?

Thanks,
Richard.

2016-04-01  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);
+ }
  \f
  
  
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	(working copy)
@@ -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()
+{
+  char a[sizeof (long)];
+  if (f ((int *)a, (long *)a) != 0)
+    abort ();
+  return 0;
+}

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

[Jakub Jelinek]

On Fri, Apr 01, 2016 at 11:08:09AM +0200, Richard Biener wrote:
> 
> RTL DSE uses true_dependence to see whether a store may be killed by
> anothe store - that's obviously broken.  The following patch makes
> it use output_dependence instead (introducing a canon_ variant of that).

I think it would be interesting to see some stats on what effect does this
have on the optimization RTL DSE is doing (say gather during
unpatched bootstrap/regtest number of successfully optimized replace_read
calls, and the same with patched bootstrap/regtest).
From quick look at the patch, this wouldn't optimize even the cases that
could be optimized (return *pi;) at the RTL level.  If the statistics would
show this affects it significantly, perhaps we could do both
canon_true_dependence and canon_output_dependence, and if the two calls
differ, don't clear the rhs, but mark it somehow and then in replace_read
check what alias set is used for the read or something similar?

	Jakub

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

[Richard Biener]

On Fri, 1 Apr 2016, Jakub Jelinek wrote:

> On Fri, Apr 01, 2016 at 11:08:09AM +0200, Richard Biener wrote:
> > 
> > RTL DSE uses true_dependence to see whether a store may be killed by
> > anothe store - that's obviously broken.  The following patch makes
> > it use output_dependence instead (introducing a canon_ variant of that).
> 
> I think it would be interesting to see some stats on what effect does this
> have on the optimization RTL DSE is doing (say gather during
> unpatched bootstrap/regtest number of successfully optimized replace_read
> calls, and the same with patched bootstrap/regtest).
> From quick look at the patch, this wouldn't optimize even the cases that
> could be optimized (return *pi;) at the RTL level.  If the statistics would
> show this affects it significantly, perhaps we could do both
> canon_true_dependence and canon_output_dependence, and if the two calls
> differ, don't clear the rhs, but mark it somehow and then in replace_read
> check what alias set is used for the read or something similar?

Well, I don't believe it is DSEs job to do CSE.  And I don't see how
we can efficiently do what you suggest - it seems DSE doesn't check
all possible aliases when CSEing.

But of course I didn't try to understand how it works and thus happily
defer to somebody else coming up with a better fix.  Maybe the correct
fix is to

      while (i_ptr)
        {
          bool remove = false;
          store_info *store_info = i_ptr->store_rec;

          /* Skip the clobbers.  */
          while (!store_info->is_set)
            store_info = store_info->next;
...

              /* If this read is just reading back something that we just
                 stored, rewrite the read.  */
              else
                {
                  if (store_info->rhs
                      && offset >= store_info->begin
                      && offset + width <= store_info->end
                      && all_positions_needed_p (store_info,
                                                 offset - 
store_info->begin,
                                                 width)
                      && replace_read (store_info, i_ptr, read_info,
                                       insn_info, loc, 
bb_info->regs_live))
                    return;

                  /* The bases are the same, just see if the offsets
                     overlap.  */
                  if ((offset < store_info->end)
                      && (offset + width > store_info->begin))
                    remove = true;

which lacks sth like a canon_true_dependence check so that if
replace_read fails it doesn't try again with other stores it finds
(hopefully the store_info->next list is "properly" ordered - you'd
want to walk backwards starting from the reads - I don't think this
is what the code above does).

So it really seems to be that the code I fixed (quoted again below)
is supposed to ensure the validity of the transform as the comment
suggests.

      else if (s_info->rhs)
        /* Need to see if it is possible for this store to overwrite
           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;
            }

As said, I don't believe in a DSE algorithm doing CSE, so ...

Richard.

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

[Jakub Jelinek]

On Fri, Apr 01, 2016 at 11:44:16AM +0200, Richard Biener wrote:
> On Fri, 1 Apr 2016, Jakub Jelinek wrote:
> 
> > On Fri, Apr 01, 2016 at 11:08:09AM +0200, Richard Biener wrote:
> > > 
> > > RTL DSE uses true_dependence to see whether a store may be killed by
> > > anothe store - that's obviously broken.  The following patch makes
> > > it use output_dependence instead (introducing a canon_ variant of that).
> > 
> > I think it would be interesting to see some stats on what effect does this
> > have on the optimization RTL DSE is doing (say gather during
> > unpatched bootstrap/regtest number of successfully optimized replace_read
> > calls, and the same with patched bootstrap/regtest).

So, I've gathered the stats, with:
--- gcc/dse.c.jj	2016-03-02 10:47:25.000000000 +0100
+++ gcc/dse.c	2016-04-01 15:01:18.831249250 +0200
@@ -2047,6 +2047,11 @@ replace_read (store_info *store_info, in
 	  print_simple_rtl (dump_file, read_reg);
 	  fprintf (dump_file, "\n");
 	}
+{
+FILE *f = fopen ("/tmp/dse2", "a");
+fprintf (f, "%d %s %s\n", (int) BITS_PER_WORD, main_input_filename ? main_input_filename : "-", current_function_name ());
+fclose (f);
+}
       return true;
     }
   else
with my usual pair of rtl,yes checking bootstraps/regtests (x86_64-linux
and i686-linux, former one with ada, latter without), both without your
patch and with the patch.
Without the patch got 66555 successful replace_reads, with your patch
only 65971, that is 1% difference.  I guess that is acceptable level,
though for GCC 7 we should try to improve that (either at the GIMPLE level,
or do something better at the RTL level).

A few randomly chosen cases where we don't optimize this anymore:
32 ../../gcc/tree-ssa-structalias.c variable_info* create_variable_info_for_1(tree, const char*, bool, bool, bitmap)
32 /home/jakub/src/gcc/gcc/testsuite/gcc.c-torture/compile/pr42237.c foo
32 /home/jakub/src/gcc/gcc/testsuite/gcc.dg/pr62167.c main
32 /home/jakub/src/gcc/gcc/testsuite/gcc.target/i386/avx-vdppd-2.c do_test
32 /home/jakub/src/gcc/libstdc++-v3/testsuite/20_util/shared_ptr/cons/58659.cc std::__shared_ptr<_Tp, _Lp>::__shared_ptr(std::unique_ptr<_Up, _Ep>&&) [with _Tp1 = X; _Del = std::default_delete<X>; <template-parameter-2-3> = void; _Tp = X; __gnu_cxx::_Lock_policy _Lp = (__gnu_cxx::_Lock_policy)2u]
32 /home/jakub/src/gcc/libstdc++-v3/testsuite/28_regex/algorithms/regex_match/ecma/char/68863.cc std::__detail::_Compiler<_TraitsT>::_Compiler(std::__detail::_Compiler<_TraitsT>::_IterT, std::__detail::_Compiler<_TraitsT>::_IterT, const typename _TraitsT::locale_type&, std::__detail::_Compiler<_TraitsT>::_FlagT) [with _TraitsT = std::__cxx11::regex_traits<char>]
32 ../../../libgo/go/crypto/x509/cert_pool.go x509.CreateCertificateRequest
64 ../../gcc/gimple-streamer-in.c void input_bb(lto_input_block*, LTO_tags, data_in*, function*, int)
64 /home/jakub/src/gcc/gcc/ada/switch-m.adb Switch.M.Normalize_Compiler_Switches.Add_Switch_Component
64 /home/jakub/src/gcc/gcc/testsuite/gcc.target/i386/sse4_1-dppd-2.c do_test
64 /home/jakub/src/gcc/libstdc++-v3/testsuite/23_containers/unordered_map/allocator/copy_assign.cc void std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _H1, _H2, _Hash, _RehashPolicy, _Traits>::_M_assign(const std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _H1, _H2, _Hash, _RehashPolicy, _Traits>&, const _NodeGenerator&) [with _NodeGenerator = std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _H1, _H2, _Hash, _RehashPolicy, _Traits>::operator=(const std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _H1, _H2, _Hash, _RehashPolicy, _Traits>&) [with _Key = T; _Value = std::pair<const T, T>; _Alloc = __gnu_test::propagating_allocator<T, true>; _ExtractKey = std::__detail::_Select1st; _Equal = equal_to; _H1 = hash; _H2 = std::__detail::_Mod_range_hashing; _Hash = std::__detail::_Default_ranged_hash; _RehashPolicy = std::__detail::_Prime_rehash_policy; _Traits = std::__detail::_Hashtable_traits<false, false, true>]::<lambda(const __node_type*)>; _Key = T; _Value = std::pair<const T, T>; _Alloc = __gnu_test::propagating_allocator<T, true>; _ExtractKey = std::__detail::_Select1st; _Equal = equal_to; _H1 = hash; _H2 = std::__detail::_Mod_range_hashing; _Hash = std::__detail::_Default_ranged_hash; _RehashPolicy = std::__detail::_Prime_rehash_policy; _Traits = std::__detail::_Hashtable_traits<false, false, true>]

	Jakub

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

[Bernd Schmidt]

On 04/01/2016 05:05 PM, Jakub Jelinek wrote:
> with my usual pair of rtl,yes checking bootstraps/regtests (x86_64-linux
> and i686-linux, former one with ada, latter without), both without your
> patch and with the patch.
> Without the patch got 66555 successful replace_reads, with your patch
> only 65971, that is 1% difference.  I guess that is acceptable level,
> though for GCC 7 we should try to improve that (either at the GIMPLE level,
> or do something better at the RTL level).

I was also running before/after tests on my set of .i files, and so far 
the effect seems negligible.


Bernd

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

[Bernd Schmidt]

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.

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?


Bernd

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

[Jeff Law]

On 04/01/2016 03:44 AM, Richard Biener wrote:
> On Fri, 1 Apr 2016, Jakub Jelinek wrote:
>
>> On Fri, Apr 01, 2016 at 11:08:09AM +0200, Richard Biener wrote:
>>>
>>> RTL DSE uses true_dependence to see whether a store may be killed by
>>> anothe store - that's obviously broken.  The following patch makes
>>> it use output_dependence instead (introducing a canon_ variant of that).
>>
>> I think it would be interesting to see some stats on what effect does this
>> have on the optimization RTL DSE is doing (say gather during
>> unpatched bootstrap/regtest number of successfully optimized replace_read
>> calls, and the same with patched bootstrap/regtest).
>>  From quick look at the patch, this wouldn't optimize even the cases that
>> could be optimized (return *pi;) at the RTL level.  If the statistics would
>> show this affects it significantly, perhaps we could do both
>> canon_true_dependence and canon_output_dependence, and if the two calls
>> differ, don't clear the rhs, but mark it somehow and then in replace_read
>> check what alias set is used for the read or something similar?
>
> Well, I don't believe it is DSEs job to do CSE.  And I don't see how
> we can efficiently do what you suggest - it seems DSE doesn't check
> all possible aliases when CSEing.
IIRC, there's nowhere else we're making this kind of transformation and 
it fits in fairly naturally in the formulations I've looked at in the 
past.

It may also be the case that this was carried forward from the old DSE 
code to prevent regressions.  I'd have to do a lot more digging in the 
archives to know for sure -- what's in dse.c doesn't look anything like 
the old DSE implementation I was familiar with.

I've often speculated that all this stuff should be rewritten using the 
scheduler's dependency framework.  Essentially when store gets scheduled 
we ought to be able to identify the potential set of dead stores, 
redundant loads and potential aliasing loads/stores as the store's 
dependencies are resolved.

I wouldn't suggest doing it as part of the scheduling pass, but instead 
as a separate pass that utilizes all the scheduler's dependency 
analysis, queue management, etc.

Any regressions (in terms of stores not eliminated or loads not CSE'd) 
would likely be inaccurate (overly conservative) dataflow build by the 
scheduler and fixing those would help both the DSE bits as well as 
scheduling in general.


I'd first look to fix the CSE-ish transformation, but if that proves 
difficult/expensive, then we'd be looking at removal.  As part of the 
removal we should extract some testcases where it correctly fired and 
create a regression bug with those testcases.

Jeff

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

[Richard Biener]

On April 1, 2016 5:26:21 PM GMT+02:00, Bernd Schmidt <bschmidt@redhat.com> 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.
>
>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?

It doesn't bypass aliasing rules but instead stores change the dynamic type of memory.

But the tests case is invalid for reasons of alignment, I'll adjust it accordingly before committing.

Richard.

>
>Bernd

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

[Richard Biener]

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);
+ }
  \f
  
  
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;
+ }

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

[Richard Biener]

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);
> + }
>   \f
>   
>   
> 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)

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

[Jakub Jelinek]

On Mon, Apr 04, 2016 at 11:24:41AM +0200, 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?

I believe all of the spill_alias_set stuff is dead for many years.
In 4.4, a call to dse_record_singleton_alias_set has been removed
(supposedly related to introduction of IRA).  Then in 4.8 you've
removed the dse_record_singleton_alias_set function, later on Lawrence
removed other small bits of this.
E.g. the alias_set_out argument from canon_address, all of spill_alias_set
handling, alias_set field, clear_alias_set_lookup, clear_alias_mode_holder,
clear_alias_group, clear_alias_mode_table are all dead IMHO.

	Jakub

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

[Richard Biener]

On Mon, 4 Apr 2016, Jakub Jelinek wrote:

> On Mon, Apr 04, 2016 at 11:24:41AM +0200, 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?
> 
> I believe all of the spill_alias_set stuff is dead for many years.
> In 4.4, a call to dse_record_singleton_alias_set has been removed
> (supposedly related to introduction of IRA).  Then in 4.8 you've
> removed the dse_record_singleton_alias_set function, later on Lawrence
> removed other small bits of this.
> E.g. the alias_set_out argument from canon_address, all of spill_alias_set
> handling, alias_set field, clear_alias_set_lookup, clear_alias_mode_holder,
> clear_alias_group, clear_alias_mode_table are all dead IMHO.

True.  By simple constant propagation I can remove a lot of code.

I'm going to bootstrap / test the following - is this ok for trunk
now (I'm going to write a better changelog).

Thanks,
Richard.

2016-04-05  Richard Biener  <rguenther@suse.de>

	* dse.c:  Remove dead code.

Index: gcc/dse.c
===================================================================
--- gcc/dse.c	(revision 234736)
+++ gcc/dse.c	(working copy)
@@ -242,9 +242,6 @@ struct store_info
   /* Canonized MEM address for use by canon_true_dependence.  */
   rtx mem_addr;
 
-  /* If this is non-zero, it is the alias set of a spill location.  */
-  alias_set_type alias_set;
-
   /* The offset of the first and byte before the last byte associated
      with the operation.  */
   HOST_WIDE_INT begin, end;
@@ -306,9 +303,6 @@ struct read_info_type
   /* The id of the mem group of the base address.  */
   int group_id;
 
-  /* If this is non-zero, it is the alias set of a spill location.  */
-  alias_set_type alias_set;
-
   /* The offset of the first and byte after the last byte associated
      with the operation.  If begin == end == 0, the read did not have
      a constant offset.  */
@@ -576,19 +570,6 @@ static object_allocator<deferred_change>
 
 static deferred_change *deferred_change_list = NULL;
 
-/* The group that holds all of the clear_alias_sets.  */
-static group_info *clear_alias_group;
-
-/* The modes of the clear_alias_sets.  */
-static htab_t clear_alias_mode_table;
-
-/* Hash table element to look up the mode for an alias set.  */
-struct clear_alias_mode_holder
-{
-  alias_set_type alias_set;
-  machine_mode mode;
-};
-
 /* This is true except if cfun->stdarg -- i.e. we cannot do
    this for vararg functions because they play games with the frame.  */
 static bool stores_off_frame_dead_at_return;
@@ -596,7 +577,6 @@ static bool stores_off_frame_dead_at_ret
 /* Counter for stats.  */
 static int globally_deleted;
 static int locally_deleted;
-static int spill_deleted;
 
 static bitmap all_blocks;
 
@@ -613,22 +593,6 @@ static unsigned int current_position;
 ----------------------------------------------------------------------------*/
 
 
-/* Find the entry associated with ALIAS_SET.  */
-
-static struct clear_alias_mode_holder *
-clear_alias_set_lookup (alias_set_type alias_set)
-{
-  struct clear_alias_mode_holder tmp_holder;
-  void **slot;
-
-  tmp_holder.alias_set = alias_set;
-  slot = htab_find_slot (clear_alias_mode_table, &tmp_holder, NO_INSERT);
-  gcc_assert (*slot);
-
-  return (struct clear_alias_mode_holder *) *slot;
-}
-
-
 /* Hashtable callbacks for maintaining the "bases" field of
    store_group_info, given that the addresses are function invariants.  */
 
@@ -665,37 +629,13 @@ get_group_info (rtx base)
   group_info *gi;
   group_info **slot;
 
-  if (base)
-    {
-      /* Find the store_base_info structure for BASE, creating a new one
-	 if necessary.  */
-      tmp_gi.rtx_base = base;
-      slot = rtx_group_table->find_slot (&tmp_gi, INSERT);
-      gi = *slot;
-    }
-  else
-    {
-      if (!clear_alias_group)
-	{
-	  clear_alias_group = gi = group_info_pool.allocate ();
-	  memset (gi, 0, sizeof (struct group_info));
-	  gi->id = rtx_group_next_id++;
-	  gi->store1_n = BITMAP_ALLOC (&dse_bitmap_obstack);
-	  gi->store1_p = BITMAP_ALLOC (&dse_bitmap_obstack);
-	  gi->store2_n = BITMAP_ALLOC (&dse_bitmap_obstack);
-	  gi->store2_p = BITMAP_ALLOC (&dse_bitmap_obstack);
-	  gi->escaped_p = BITMAP_ALLOC (&dse_bitmap_obstack);
-	  gi->escaped_n = BITMAP_ALLOC (&dse_bitmap_obstack);
-	  gi->group_kill = BITMAP_ALLOC (&dse_bitmap_obstack);
-	  gi->process_globally = false;
-	  gi->offset_map_size_n = 0;
-	  gi->offset_map_size_p = 0;
-	  gi->offset_map_n = NULL;
-	  gi->offset_map_p = NULL;
-	  rtx_group_vec.safe_push (gi);
-	}
-      return clear_alias_group;
-    }
+  gcc_assert (base != NULL_RTX);
+
+  /* Find the store_base_info structure for BASE, creating a new one
+     if necessary.  */
+  tmp_gi.rtx_base = base;
+  slot = rtx_group_table->find_slot (&tmp_gi, INSERT);
+  gi = *slot;
 
   if (gi == NULL)
     {
@@ -732,7 +672,6 @@ dse_step0 (void)
 {
   locally_deleted = 0;
   globally_deleted = 0;
-  spill_deleted = 0;
 
   bitmap_obstack_initialize (&dse_bitmap_obstack);
   gcc_obstack_init (&dse_obstack);
@@ -749,8 +688,6 @@ dse_step0 (void)
   stores_off_frame_dead_at_return = !cfun->stdarg;
 
   init_alias_analysis ();
-
-  clear_alias_group = NULL;
 }
 
 
@@ -919,15 +856,8 @@ delete_dead_store_insn (insn_info_t insn
   if (!check_for_inc_dec_1 (insn_info))
     return;
   if (dump_file && (dump_flags & TDF_DETAILS))
-    {
-      fprintf (dump_file, "Locally deleting insn %d ",
-	       INSN_UID (insn_info->insn));
-      if (insn_info->store_rec->alias_set)
-	fprintf (dump_file, "alias set %d\n",
-		 (int) insn_info->store_rec->alias_set);
-      else
-	fprintf (dump_file, "\n");
-    }
+    fprintf (dump_file, "Locally deleting insn %d\n",
+	     INSN_UID (insn_info->insn));
 
   free_store_info (insn_info);
   read_info = insn_info->read_rec;
@@ -1057,13 +987,8 @@ free_read_records (bb_info_t bb_info)
   while (*ptr)
     {
       read_info_t next = (*ptr)->next;
-      if ((*ptr)->alias_set == 0)
-        {
-	  read_info_type_pool.remove (*ptr);
-          *ptr = next;
-        }
-      else
-        ptr = &(*ptr)->next;
+      read_info_type_pool.remove (*ptr);
+      *ptr = next;
     }
 }
 
@@ -1137,7 +1062,6 @@ const_or_frame_p (rtx x)
 
 static bool
 canon_address (rtx mem,
-	       alias_set_type *alias_set_out,
 	       int *group_id,
 	       HOST_WIDE_INT *offset,
 	       cselib_val **base)
@@ -1147,8 +1071,6 @@ canon_address (rtx mem,
   rtx expanded_address, address;
   int expanded;
 
-  *alias_set_out = 0;
-
   cselib_lookup (mem_address, address_mode, 1, GET_MODE (mem));
 
   if (dump_file && (dump_flags & TDF_DETAILS))
@@ -1347,7 +1269,6 @@ record_store (rtx body, bb_info_t bb_inf
   rtx mem, rhs, const_rhs, mem_addr;
   HOST_WIDE_INT offset = 0;
   HOST_WIDE_INT width = 0;
-  alias_set_type spill_alias_set;
   insn_info_t insn_info = bb_info->last_insn;
   store_info *store_info = NULL;
   int group_id;
@@ -1410,7 +1331,7 @@ record_store (rtx body, bb_info_t bb_inf
   if (MEM_VOLATILE_P (mem))
     insn_info->cannot_delete = true;
 
-  if (!canon_address (mem, &spill_alias_set, &group_id, &offset, &base))
+  if (!canon_address (mem, &group_id, &offset, &base))
     {
       clear_rhs_from_active_local_stores ();
       return 0;
@@ -1421,26 +1342,7 @@ record_store (rtx body, bb_info_t bb_inf
   else
     width = GET_MODE_SIZE (GET_MODE (mem));
 
-  if (spill_alias_set)
-    {
-      bitmap store1 = clear_alias_group->store1_p;
-      bitmap store2 = clear_alias_group->store2_p;
-
-      gcc_assert (GET_MODE (mem) != BLKmode);
-
-      if (!bitmap_set_bit (store1, spill_alias_set))
-	bitmap_set_bit (store2, spill_alias_set);
-
-      if (clear_alias_group->offset_map_size_p < spill_alias_set)
-	clear_alias_group->offset_map_size_p = spill_alias_set;
-
-      store_info = rtx_store_info_pool.allocate ();
-
-      if (dump_file && (dump_flags & TDF_DETAILS))
-	fprintf (dump_file, " processing spill store %d(%s)\n",
-		 (int) spill_alias_set, GET_MODE_NAME (GET_MODE (mem)));
-    }
-  else if (group_id >= 0)
+  if (group_id >= 0)
     {
       /* In the restrictive case where the base is a constant or the
 	 frame pointer we can do global analysis.  */
@@ -1506,21 +1408,16 @@ record_store (rtx body, bb_info_t bb_inf
   last = NULL;
   redundant_reason = NULL;
   mem = canon_rtx (mem);
-  /* For alias_set != 0 canon_true_dependence should be never called.  */
-  if (spill_alias_set)
-    mem_addr = NULL_RTX;
+
+  if (group_id < 0)
+    mem_addr = base->val_rtx;
   else
     {
-      if (group_id < 0)
-	mem_addr = base->val_rtx;
-      else
-	{
-	  group_info *group = rtx_group_vec[group_id];
-	  mem_addr = group->canon_base_addr;
-	}
-      if (offset)
-	mem_addr = plus_constant (get_address_mode (mem), mem_addr, offset);
+      group_info *group = rtx_group_vec[group_id];
+      mem_addr = group->canon_base_addr;
     }
+  if (offset)
+    mem_addr = plus_constant (get_address_mode (mem), mem_addr, offset);
 
   while (ptr)
     {
@@ -1534,29 +1431,8 @@ record_store (rtx body, bb_info_t bb_inf
       while (!s_info->is_set)
 	s_info = s_info->next;
 
-      if (s_info->alias_set != spill_alias_set)
-	del = false;
-      else if (s_info->alias_set)
-	{
-	  struct clear_alias_mode_holder *entry
-	    = clear_alias_set_lookup (s_info->alias_set);
-	  /* Generally, spills cannot be processed if and of the
-	     references to the slot have a different mode.  But if
-	     we are in the same block and mode is exactly the same
-	     between this store and one before in the same block,
-	     we can still delete it.  */
-	  if ((GET_MODE (mem) == GET_MODE (s_info->mem))
-	      && (GET_MODE (mem) == entry->mode))
-	    {
-	      del = true;
-	      set_all_positions_unneeded (s_info);
-	    }
-	  if (dump_file && (dump_flags & TDF_DETAILS))
-	    fprintf (dump_file, "    trying spill store in insn=%d alias_set=%d\n",
-		     INSN_UID (ptr->insn), (int) s_info->alias_set);
-	}
-      else if ((s_info->group_id == group_id)
-	       && (s_info->cse_base == base))
+      if ((s_info->group_id == group_id)
+	  && (s_info->cse_base == base))
 	{
 	  HOST_WIDE_INT i;
 	  if (dump_file && (dump_flags & TDF_DETAILS))
@@ -1646,7 +1522,6 @@ record_store (rtx body, bb_info_t bb_inf
   store_info->next = insn_info->store_rec;
   insn_info->store_rec = store_info;
   store_info->mem = mem;
-  store_info->alias_set = spill_alias_set;
   store_info->mem_addr = mem_addr;
   store_info->cse_base = base;
   if (width > HOST_BITS_PER_WIDE_INT)
@@ -2070,7 +1945,6 @@ check_mem_read_rtx (rtx *loc, bb_info_t
   insn_info_t insn_info;
   HOST_WIDE_INT offset = 0;
   HOST_WIDE_INT width = 0;
-  alias_set_type spill_alias_set = 0;
   cselib_val *base = NULL;
   int group_id;
   read_info_t read_info;
@@ -2092,7 +1966,7 @@ check_mem_read_rtx (rtx *loc, bb_info_t
   if (MEM_READONLY_P (mem))
     return;
 
-  if (!canon_address (mem, &spill_alias_set, &group_id, &offset, &base))
+  if (!canon_address (mem, &group_id, &offset, &base))
     {
       if (dump_file && (dump_flags & TDF_DETAILS))
 	fprintf (dump_file, " adding wild read, canon_address failure.\n");
@@ -2108,64 +1982,21 @@ check_mem_read_rtx (rtx *loc, bb_info_t
   read_info = read_info_type_pool.allocate ();
   read_info->group_id = group_id;
   read_info->mem = mem;
-  read_info->alias_set = spill_alias_set;
   read_info->begin = offset;
   read_info->end = offset + width;
   read_info->next = insn_info->read_rec;
   insn_info->read_rec = read_info;
-  /* For alias_set != 0 canon_true_dependence should be never called.  */
-  if (spill_alias_set)
-    mem_addr = NULL_RTX;
+  if (group_id < 0)
+    mem_addr = base->val_rtx;
   else
     {
-      if (group_id < 0)
-	mem_addr = base->val_rtx;
-      else
-	{
-	  group_info *group = rtx_group_vec[group_id];
-	  mem_addr = group->canon_base_addr;
-	}
-      if (offset)
-	mem_addr = plus_constant (get_address_mode (mem), mem_addr, offset);
+      group_info *group = rtx_group_vec[group_id];
+      mem_addr = group->canon_base_addr;
     }
+  if (offset)
+    mem_addr = plus_constant (get_address_mode (mem), mem_addr, offset);
 
-  /* We ignore the clobbers in store_info.  The is mildly aggressive,
-     but there really should not be a clobber followed by a read.  */
-
-  if (spill_alias_set)
-    {
-      insn_info_t i_ptr = active_local_stores;
-      insn_info_t last = NULL;
-
-      if (dump_file && (dump_flags & TDF_DETAILS))
-	fprintf (dump_file, " processing spill load %d\n",
-		 (int) spill_alias_set);
-
-      while (i_ptr)
-	{
-	  store_info *store_info = i_ptr->store_rec;
-
-	  /* Skip the clobbers.  */
-	  while (!store_info->is_set)
-	    store_info = store_info->next;
-
-	  if (store_info->alias_set == spill_alias_set)
-	    {
-	      if (dump_file && (dump_flags & TDF_DETAILS))
-		dump_insn_info ("removing from active", i_ptr);
-
-	      active_local_stores_len--;
-	      if (last)
-		last->next_local_store = i_ptr->next_local_store;
-	      else
-		active_local_stores = i_ptr->next_local_store;
-	    }
-	  else
-	    last = i_ptr;
-	  i_ptr = i_ptr->next_local_store;
-	}
-    }
-  else if (group_id >= 0)
+  if (group_id >= 0)
     {
       /* This is the restricted case where the base is a constant or
 	 the frame pointer and offset is a constant.  */
@@ -2293,11 +2124,10 @@ check_mem_read_rtx (rtx *loc, bb_info_t
 			       bb_info->regs_live))
 	    return;
 
-	  if (!store_info->alias_set)
-	    remove = canon_true_dependence (store_info->mem,
-					    GET_MODE (store_info->mem),
-					    store_info->mem_addr,
-					    mem, mem_addr);
+	  remove = canon_true_dependence (store_info->mem,
+					  GET_MODE (store_info->mem),
+					  store_info->mem_addr,
+					  mem, mem_addr);
 
 	  if (remove)
 	    {
@@ -2730,16 +2560,13 @@ dse_step1 (void)
 		  /* Skip the clobbers.  */
 		  while (!store_info->is_set)
 		    store_info = store_info->next;
-		  if (store_info->alias_set && !i_ptr->cannot_delete)
-		    delete_dead_store_insn (i_ptr);
-		  else
-		    if (store_info->group_id >= 0)
-		      {
-			group_info *group
+		  if (store_info->group_id >= 0)
+		    {
+		      group_info *group
 			  = rtx_group_vec[store_info->group_id];
-			if (group->frame_related && !i_ptr->cannot_delete)
-			  delete_dead_store_insn (i_ptr);
-		      }
+		      if (group->frame_related && !i_ptr->cannot_delete)
+			delete_dead_store_insn (i_ptr);
+		    }
 
 		  i_ptr = i_ptr->next_local_store;
 		}
@@ -2883,9 +2710,6 @@ dse_step2_nospill (void)
       bitmap_iterator bi;
       unsigned int j;
 
-      if (group == clear_alias_group)
-	continue;
-
       memset (group->offset_map_n, 0, sizeof (int) * group->offset_map_size_n);
       memset (group->offset_map_p, 0, sizeof (int) * group->offset_map_size_p);
       bitmap_clear (group->group_kill);
@@ -2968,30 +2792,6 @@ scan_stores_nospill (store_info *store_i
 }
 
 
-/* Process the STORE_INFOs into the bitmaps into GEN and KILL.  KILL
-   may be NULL. */
-
-static void
-scan_stores_spill (store_info *store_info, bitmap gen, bitmap kill)
-{
-  while (store_info)
-    {
-      if (store_info->alias_set)
-	{
-	  int index = get_bitmap_index (clear_alias_group,
-					store_info->alias_set);
-	  if (index != 0)
-	    {
-	      bitmap_set_bit (gen, index);
-	      if (kill)
-		bitmap_clear_bit (kill, index);
-	    }
-	}
-      store_info = store_info->next;
-    }
-}
-
-
 /* Process the READ_INFOs into the bitmaps into GEN and KILL.  KILL
    may be NULL.  */
 
@@ -3086,30 +2886,6 @@ scan_reads_nospill (insn_info_t insn_inf
     }
 }
 
-/* Process the READ_INFOs into the bitmaps into GEN and KILL.  KILL
-   may be NULL.  */
-
-static void
-scan_reads_spill (read_info_t read_info, bitmap gen, bitmap kill)
-{
-  while (read_info)
-    {
-      if (read_info->alias_set)
-	{
-	  int index = get_bitmap_index (clear_alias_group,
-					read_info->alias_set);
-	  if (index != 0)
-	    {
-	      if (kill)
-		bitmap_set_bit (kill, index);
-	      bitmap_clear_bit (gen, index);
-	    }
-	}
-
-      read_info = read_info->next;
-    }
-}
-
 
 /* Return the insn in BB_INFO before the first wild read or if there
    are no wild reads in the block, return the last insn.  */
@@ -3148,16 +2924,12 @@ find_insn_before_first_wild_read (bb_inf
    anything that happens is hidden by the wild read.  */
 
 static void
-dse_step3_scan (bool for_spills, basic_block bb)
+dse_step3_scan (basic_block bb)
 {
   bb_info_t bb_info = bb_table[bb->index];
   insn_info_t insn_info;
 
-  if (for_spills)
-    /* There are no wild reads in the spill case.  */
-    insn_info = bb_info->last_insn;
-  else
-    insn_info = find_insn_before_first_wild_read (bb_info);
+  insn_info = find_insn_before_first_wild_read (bb_info);
 
   /* In the spill case or in the no_spill case if there is no wild
      read in the block, we will need a kill set.  */
@@ -3178,17 +2950,8 @@ dse_step3_scan (bool for_spills, basic_b
 	 this phase.  */
       if (insn_info->insn && INSN_P (insn_info->insn))
 	{
-	  /* Process the read(s) last.  */
-	  if (for_spills)
-	    {
-	      scan_stores_spill (insn_info->store_rec, bb_info->gen, bb_info->kill);
-	      scan_reads_spill (insn_info->read_rec, bb_info->gen, bb_info->kill);
-	    }
-	  else
-	    {
-	      scan_stores_nospill (insn_info->store_rec, bb_info->gen, bb_info->kill);
-	      scan_reads_nospill (insn_info, bb_info->gen, bb_info->kill);
-	    }
+	  scan_stores_nospill (insn_info->store_rec, bb_info->gen, bb_info->kill);
+	  scan_reads_nospill (insn_info, bb_info->gen, bb_info->kill);
 	}
 
       insn_info = insn_info->prev_insn;
@@ -3243,7 +3006,7 @@ mark_reachable_blocks (sbitmap unreachab
 /* Build the transfer functions for the function.  */
 
 static void
-dse_step3 (bool for_spills)
+dse_step3 ()
 {
   basic_block bb;
   sbitmap unreachable_blocks = sbitmap_alloc (last_basic_block_for_fn (cfun));
@@ -3266,7 +3029,7 @@ dse_step3 (bool for_spills)
       else if (bb->index == EXIT_BLOCK)
 	dse_step3_exit_block_scan (bb_info);
       else
-	dse_step3_scan (for_spills, bb);
+	dse_step3_scan (bb);
       if (EDGE_COUNT (bb->succs) == 0)
 	mark_reachable_blocks (unreachable_blocks, bb);
 
@@ -3502,27 +3265,21 @@ dse_step5_nospill (void)
 	      while (!store_info->is_set)
 		store_info = store_info->next;
 
-	      if (store_info->alias_set)
-		deleted = false;
-	      else
+	      HOST_WIDE_INT i;
+	      group_info *group_info = rtx_group_vec[store_info->group_id];
+
+	      for (i = store_info->begin; i < store_info->end; i++)
 		{
-		  HOST_WIDE_INT i;
-		  group_info *group_info
-		    = rtx_group_vec[store_info->group_id];
+		  int index = get_bitmap_index (group_info, i);
 
-		  for (i = store_info->begin; i < store_info->end; i++)
+		  if (dump_file && (dump_flags & TDF_DETAILS))
+		    fprintf (dump_file, "i = %d, index = %d\n", (int)i, index);
+		  if (index == 0 || !bitmap_bit_p (v, index))
 		    {
-		      int index = get_bitmap_index (group_info, i);
-
 		      if (dump_file && (dump_flags & TDF_DETAILS))
-			fprintf (dump_file, "i = %d, index = %d\n", (int)i, index);
-		      if (index == 0 || !bitmap_bit_p (v, index))
-			{
-			  if (dump_file && (dump_flags & TDF_DETAILS))
-			    fprintf (dump_file, "failing at i = %d\n", (int)i);
-			  deleted = false;
-			  break;
-			}
+			fprintf (dump_file, "failing at i = %d\n", (int)i);
+		      deleted = false;
+		      break;
 		    }
 		}
 	      if (deleted)
@@ -3672,7 +3429,7 @@ rest_of_handle_dse (void)
       df_analyze ();
       if (dump_file && (dump_flags & TDF_DETAILS))
 	fprintf (dump_file, "doing global processing\n");
-      dse_step3 (false);
+      dse_step3 ();
       dse_step4 ();
       dse_step5_nospill ();
     }
@@ -3681,8 +3438,8 @@ rest_of_handle_dse (void)
   dse_step7 ();
 
   if (dump_file)
-    fprintf (dump_file, "dse: local deletions = %d, global deletions = %d, spill deletions = %d\n",
-	     locally_deleted, globally_deleted, spill_deleted);
+    fprintf (dump_file, "dse: local deletions = %d, global deletions = %d\n",
+	     locally_deleted, globally_deleted);
 
   /* DSE can eliminate potentially-trapping MEMs.
      Remove any EH edges associated with them.  */

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

[Jakub Jelinek]

On Tue, Apr 05, 2016 at 11:05:44AM +0200, Richard Biener wrote:
> True.  By simple constant propagation I can remove a lot of code.
> 
> I'm going to bootstrap / test the following - is this ok for trunk
> now (I'm going to write a better changelog).

LGTM with better Changelog, though I have small nits:

> -      else if ((s_info->group_id == group_id)
> -	       && (s_info->cse_base == base))
> +      if ((s_info->group_id == group_id)
> +	  && (s_info->cse_base == base))

+      if (s_info->group_id == group_id && s_info->cse_base == base)

instead, please.

> +		  if (store_info->group_id >= 0)
> +		    {
> +		      group_info *group
>  			  = rtx_group_vec[store_info->group_id];

The formatting looks wrong and
		      group_info *group = rtx_group_vec[store_info->group_id];
fits now on one line.

> @@ -3086,30 +2886,6 @@ scan_reads_nospill (insn_info_t insn_inf

Please also rename the
dse_step2_nospill
scan_stores_nospill
scan_reads_nospill
dse_step5_nospill
functions to s/_nospill//g and adjust all their uses (and perhaps function
comments).

	Jakub

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

[Richard Biener]

On Tue, 5 Apr 2016, Jakub Jelinek wrote:

> On Tue, Apr 05, 2016 at 11:05:44AM +0200, Richard Biener wrote:
> > True.  By simple constant propagation I can remove a lot of code.
> > 
> > I'm going to bootstrap / test the following - is this ok for trunk
> > now (I'm going to write a better changelog).
> 
> LGTM with better Changelog, though I have small nits:
> 
> > -      else if ((s_info->group_id == group_id)
> > -	       && (s_info->cse_base == base))
> > +      if ((s_info->group_id == group_id)
> > +	  && (s_info->cse_base == base))
> 
> +      if (s_info->group_id == group_id && s_info->cse_base == base)
> 
> instead, please.
> 
> > +		  if (store_info->group_id >= 0)
> > +		    {
> > +		      group_info *group
> >  			  = rtx_group_vec[store_info->group_id];
> 
> The formatting looks wrong and
> 		      group_info *group = rtx_group_vec[store_info->group_id];
> fits now on one line.
> 
> > @@ -3086,30 +2886,6 @@ scan_reads_nospill (insn_info_t insn_inf
> 
> Please also rename the
> dse_step2_nospill
> scan_stores_nospill
> scan_reads_nospill
> dse_step5_nospill
> functions to s/_nospill//g and adjust all their uses (and perhaps function
> comments).

Like this?

Thanks,
Richard.

2016-04-05  Richard Biener  <rguenther@suse.de>

	* dse.c (struct store_info): Remove alias_set member.
	(struct read_info_type): Likewise.
	(clear_alias_group, clear_alias_mode_table, clear_alias_mode_holder,
	spill_deleted, clear_alias_set_lookup): Remove.
	(get_group_info): Remove dead base == NULL_RTX case.
	(dse_step0): Remove initialization of removed variables.
	(delete_dead_store_insn): Reomve alias set dumping.
	(free_read_records): Remove alias_set handling.
	(canon_address): Remove alias_set_out parameter.
	(record_store): Remove spill_alias_set, it's always zero.
	(check_mem_read_rtx): Likewise.
	(dse_step2): Rename from ...
	(dse_step2_nospill): ... this.  Adjust.
	(scan_stores): Rename from ...
	(scan_stores_nospill): ... this.
	(scan_reads): Rename from ...
	(scan_reads_nospill): ... this.
	(scan_stores_spill, scan_reads_spill): Remove.
	(dse_step3_scan): Remove for_spills argument which is always false.
	(dse_step3): Likewise.
	(dse_step5): Rename from ...
	(dse_step5_nospill): ... this.  Remove alias_set handling.
	(rest_of_handle_dse): Adjust.

Index: gcc/dse.c
===================================================================
--- gcc/dse.c	(revision 234736)
+++ gcc/dse.c	(working copy)
@@ -242,9 +242,6 @@ struct store_info
   /* Canonized MEM address for use by canon_true_dependence.  */
   rtx mem_addr;
 
-  /* If this is non-zero, it is the alias set of a spill location.  */
-  alias_set_type alias_set;
-
   /* The offset of the first and byte before the last byte associated
      with the operation.  */
   HOST_WIDE_INT begin, end;
@@ -306,9 +303,6 @@ struct read_info_type
   /* The id of the mem group of the base address.  */
   int group_id;
 
-  /* If this is non-zero, it is the alias set of a spill location.  */
-  alias_set_type alias_set;
-
   /* The offset of the first and byte after the last byte associated
      with the operation.  If begin == end == 0, the read did not have
      a constant offset.  */
@@ -576,19 +570,6 @@ static object_allocator<deferred_change>
 
 static deferred_change *deferred_change_list = NULL;
 
-/* The group that holds all of the clear_alias_sets.  */
-static group_info *clear_alias_group;
-
-/* The modes of the clear_alias_sets.  */
-static htab_t clear_alias_mode_table;
-
-/* Hash table element to look up the mode for an alias set.  */
-struct clear_alias_mode_holder
-{
-  alias_set_type alias_set;
-  machine_mode mode;
-};
-
 /* This is true except if cfun->stdarg -- i.e. we cannot do
    this for vararg functions because they play games with the frame.  */
 static bool stores_off_frame_dead_at_return;
@@ -596,7 +577,6 @@ static bool stores_off_frame_dead_at_ret
 /* Counter for stats.  */
 static int globally_deleted;
 static int locally_deleted;
-static int spill_deleted;
 
 static bitmap all_blocks;
 
@@ -613,22 +593,6 @@ static unsigned int current_position;
 ----------------------------------------------------------------------------*/
 
 
-/* Find the entry associated with ALIAS_SET.  */
-
-static struct clear_alias_mode_holder *
-clear_alias_set_lookup (alias_set_type alias_set)
-{
-  struct clear_alias_mode_holder tmp_holder;
-  void **slot;
-
-  tmp_holder.alias_set = alias_set;
-  slot = htab_find_slot (clear_alias_mode_table, &tmp_holder, NO_INSERT);
-  gcc_assert (*slot);
-
-  return (struct clear_alias_mode_holder *) *slot;
-}
-
-
 /* Hashtable callbacks for maintaining the "bases" field of
    store_group_info, given that the addresses are function invariants.  */
 
@@ -665,37 +629,13 @@ get_group_info (rtx base)
   group_info *gi;
   group_info **slot;
 
-  if (base)
-    {
-      /* Find the store_base_info structure for BASE, creating a new one
-	 if necessary.  */
-      tmp_gi.rtx_base = base;
-      slot = rtx_group_table->find_slot (&tmp_gi, INSERT);
-      gi = *slot;
-    }
-  else
-    {
-      if (!clear_alias_group)
-	{
-	  clear_alias_group = gi = group_info_pool.allocate ();
-	  memset (gi, 0, sizeof (struct group_info));
-	  gi->id = rtx_group_next_id++;
-	  gi->store1_n = BITMAP_ALLOC (&dse_bitmap_obstack);
-	  gi->store1_p = BITMAP_ALLOC (&dse_bitmap_obstack);
-	  gi->store2_n = BITMAP_ALLOC (&dse_bitmap_obstack);
-	  gi->store2_p = BITMAP_ALLOC (&dse_bitmap_obstack);
-	  gi->escaped_p = BITMAP_ALLOC (&dse_bitmap_obstack);
-	  gi->escaped_n = BITMAP_ALLOC (&dse_bitmap_obstack);
-	  gi->group_kill = BITMAP_ALLOC (&dse_bitmap_obstack);
-	  gi->process_globally = false;
-	  gi->offset_map_size_n = 0;
-	  gi->offset_map_size_p = 0;
-	  gi->offset_map_n = NULL;
-	  gi->offset_map_p = NULL;
-	  rtx_group_vec.safe_push (gi);
-	}
-      return clear_alias_group;
-    }
+  gcc_assert (base != NULL_RTX);
+
+  /* Find the store_base_info structure for BASE, creating a new one
+     if necessary.  */
+  tmp_gi.rtx_base = base;
+  slot = rtx_group_table->find_slot (&tmp_gi, INSERT);
+  gi = *slot;
 
   if (gi == NULL)
     {
@@ -732,7 +672,6 @@ dse_step0 (void)
 {
   locally_deleted = 0;
   globally_deleted = 0;
-  spill_deleted = 0;
 
   bitmap_obstack_initialize (&dse_bitmap_obstack);
   gcc_obstack_init (&dse_obstack);
@@ -749,8 +688,6 @@ dse_step0 (void)
   stores_off_frame_dead_at_return = !cfun->stdarg;
 
   init_alias_analysis ();
-
-  clear_alias_group = NULL;
 }
 
 
@@ -919,15 +856,8 @@ delete_dead_store_insn (insn_info_t insn
   if (!check_for_inc_dec_1 (insn_info))
     return;
   if (dump_file && (dump_flags & TDF_DETAILS))
-    {
-      fprintf (dump_file, "Locally deleting insn %d ",
-	       INSN_UID (insn_info->insn));
-      if (insn_info->store_rec->alias_set)
-	fprintf (dump_file, "alias set %d\n",
-		 (int) insn_info->store_rec->alias_set);
-      else
-	fprintf (dump_file, "\n");
-    }
+    fprintf (dump_file, "Locally deleting insn %d\n",
+	     INSN_UID (insn_info->insn));
 
   free_store_info (insn_info);
   read_info = insn_info->read_rec;
@@ -1057,13 +987,8 @@ free_read_records (bb_info_t bb_info)
   while (*ptr)
     {
       read_info_t next = (*ptr)->next;
-      if ((*ptr)->alias_set == 0)
-        {
-	  read_info_type_pool.remove (*ptr);
-          *ptr = next;
-        }
-      else
-        ptr = &(*ptr)->next;
+      read_info_type_pool.remove (*ptr);
+      *ptr = next;
     }
 }
 
@@ -1137,7 +1062,6 @@ const_or_frame_p (rtx x)
 
 static bool
 canon_address (rtx mem,
-	       alias_set_type *alias_set_out,
 	       int *group_id,
 	       HOST_WIDE_INT *offset,
 	       cselib_val **base)
@@ -1147,8 +1071,6 @@ canon_address (rtx mem,
   rtx expanded_address, address;
   int expanded;
 
-  *alias_set_out = 0;
-
   cselib_lookup (mem_address, address_mode, 1, GET_MODE (mem));
 
   if (dump_file && (dump_flags & TDF_DETAILS))
@@ -1347,7 +1269,6 @@ record_store (rtx body, bb_info_t bb_inf
   rtx mem, rhs, const_rhs, mem_addr;
   HOST_WIDE_INT offset = 0;
   HOST_WIDE_INT width = 0;
-  alias_set_type spill_alias_set;
   insn_info_t insn_info = bb_info->last_insn;
   store_info *store_info = NULL;
   int group_id;
@@ -1410,7 +1331,7 @@ record_store (rtx body, bb_info_t bb_inf
   if (MEM_VOLATILE_P (mem))
     insn_info->cannot_delete = true;
 
-  if (!canon_address (mem, &spill_alias_set, &group_id, &offset, &base))
+  if (!canon_address (mem, &group_id, &offset, &base))
     {
       clear_rhs_from_active_local_stores ();
       return 0;
@@ -1421,26 +1342,7 @@ record_store (rtx body, bb_info_t bb_inf
   else
     width = GET_MODE_SIZE (GET_MODE (mem));
 
-  if (spill_alias_set)
-    {
-      bitmap store1 = clear_alias_group->store1_p;
-      bitmap store2 = clear_alias_group->store2_p;
-
-      gcc_assert (GET_MODE (mem) != BLKmode);
-
-      if (!bitmap_set_bit (store1, spill_alias_set))
-	bitmap_set_bit (store2, spill_alias_set);
-
-      if (clear_alias_group->offset_map_size_p < spill_alias_set)
-	clear_alias_group->offset_map_size_p = spill_alias_set;
-
-      store_info = rtx_store_info_pool.allocate ();
-
-      if (dump_file && (dump_flags & TDF_DETAILS))
-	fprintf (dump_file, " processing spill store %d(%s)\n",
-		 (int) spill_alias_set, GET_MODE_NAME (GET_MODE (mem)));
-    }
-  else if (group_id >= 0)
+  if (group_id >= 0)
     {
       /* In the restrictive case where the base is a constant or the
 	 frame pointer we can do global analysis.  */
@@ -1506,21 +1408,16 @@ record_store (rtx body, bb_info_t bb_inf
   last = NULL;
   redundant_reason = NULL;
   mem = canon_rtx (mem);
-  /* For alias_set != 0 canon_true_dependence should be never called.  */
-  if (spill_alias_set)
-    mem_addr = NULL_RTX;
+
+  if (group_id < 0)
+    mem_addr = base->val_rtx;
   else
     {
-      if (group_id < 0)
-	mem_addr = base->val_rtx;
-      else
-	{
-	  group_info *group = rtx_group_vec[group_id];
-	  mem_addr = group->canon_base_addr;
-	}
-      if (offset)
-	mem_addr = plus_constant (get_address_mode (mem), mem_addr, offset);
+      group_info *group = rtx_group_vec[group_id];
+      mem_addr = group->canon_base_addr;
     }
+  if (offset)
+    mem_addr = plus_constant (get_address_mode (mem), mem_addr, offset);
 
   while (ptr)
     {
@@ -1534,29 +1431,7 @@ record_store (rtx body, bb_info_t bb_inf
       while (!s_info->is_set)
 	s_info = s_info->next;
 
-      if (s_info->alias_set != spill_alias_set)
-	del = false;
-      else if (s_info->alias_set)
-	{
-	  struct clear_alias_mode_holder *entry
-	    = clear_alias_set_lookup (s_info->alias_set);
-	  /* Generally, spills cannot be processed if and of the
-	     references to the slot have a different mode.  But if
-	     we are in the same block and mode is exactly the same
-	     between this store and one before in the same block,
-	     we can still delete it.  */
-	  if ((GET_MODE (mem) == GET_MODE (s_info->mem))
-	      && (GET_MODE (mem) == entry->mode))
-	    {
-	      del = true;
-	      set_all_positions_unneeded (s_info);
-	    }
-	  if (dump_file && (dump_flags & TDF_DETAILS))
-	    fprintf (dump_file, "    trying spill store in insn=%d alias_set=%d\n",
-		     INSN_UID (ptr->insn), (int) s_info->alias_set);
-	}
-      else if ((s_info->group_id == group_id)
-	       && (s_info->cse_base == base))
+      if (s_info->group_id == group_id && s_info->cse_base == base)
 	{
 	  HOST_WIDE_INT i;
 	  if (dump_file && (dump_flags & TDF_DETAILS))
@@ -1646,7 +1521,6 @@ record_store (rtx body, bb_info_t bb_inf
   store_info->next = insn_info->store_rec;
   insn_info->store_rec = store_info;
   store_info->mem = mem;
-  store_info->alias_set = spill_alias_set;
   store_info->mem_addr = mem_addr;
   store_info->cse_base = base;
   if (width > HOST_BITS_PER_WIDE_INT)
@@ -2070,7 +1944,6 @@ check_mem_read_rtx (rtx *loc, bb_info_t
   insn_info_t insn_info;
   HOST_WIDE_INT offset = 0;
   HOST_WIDE_INT width = 0;
-  alias_set_type spill_alias_set = 0;
   cselib_val *base = NULL;
   int group_id;
   read_info_t read_info;
@@ -2092,7 +1965,7 @@ check_mem_read_rtx (rtx *loc, bb_info_t
   if (MEM_READONLY_P (mem))
     return;
 
-  if (!canon_address (mem, &spill_alias_set, &group_id, &offset, &base))
+  if (!canon_address (mem, &group_id, &offset, &base))
     {
       if (dump_file && (dump_flags & TDF_DETAILS))
 	fprintf (dump_file, " adding wild read, canon_address failure.\n");
@@ -2108,64 +1981,21 @@ check_mem_read_rtx (rtx *loc, bb_info_t
   read_info = read_info_type_pool.allocate ();
   read_info->group_id = group_id;
   read_info->mem = mem;
-  read_info->alias_set = spill_alias_set;
   read_info->begin = offset;
   read_info->end = offset + width;
   read_info->next = insn_info->read_rec;
   insn_info->read_rec = read_info;
-  /* For alias_set != 0 canon_true_dependence should be never called.  */
-  if (spill_alias_set)
-    mem_addr = NULL_RTX;
+  if (group_id < 0)
+    mem_addr = base->val_rtx;
   else
     {
-      if (group_id < 0)
-	mem_addr = base->val_rtx;
-      else
-	{
-	  group_info *group = rtx_group_vec[group_id];
-	  mem_addr = group->canon_base_addr;
-	}
-      if (offset)
-	mem_addr = plus_constant (get_address_mode (mem), mem_addr, offset);
+      group_info *group = rtx_group_vec[group_id];
+      mem_addr = group->canon_base_addr;
     }
+  if (offset)
+    mem_addr = plus_constant (get_address_mode (mem), mem_addr, offset);
 
-  /* We ignore the clobbers in store_info.  The is mildly aggressive,
-     but there really should not be a clobber followed by a read.  */
-
-  if (spill_alias_set)
-    {
-      insn_info_t i_ptr = active_local_stores;
-      insn_info_t last = NULL;
-
-      if (dump_file && (dump_flags & TDF_DETAILS))
-	fprintf (dump_file, " processing spill load %d\n",
-		 (int) spill_alias_set);
-
-      while (i_ptr)
-	{
-	  store_info *store_info = i_ptr->store_rec;
-
-	  /* Skip the clobbers.  */
-	  while (!store_info->is_set)
-	    store_info = store_info->next;
-
-	  if (store_info->alias_set == spill_alias_set)
-	    {
-	      if (dump_file && (dump_flags & TDF_DETAILS))
-		dump_insn_info ("removing from active", i_ptr);
-
-	      active_local_stores_len--;
-	      if (last)
-		last->next_local_store = i_ptr->next_local_store;
-	      else
-		active_local_stores = i_ptr->next_local_store;
-	    }
-	  else
-	    last = i_ptr;
-	  i_ptr = i_ptr->next_local_store;
-	}
-    }
-  else if (group_id >= 0)
+  if (group_id >= 0)
     {
       /* This is the restricted case where the base is a constant or
 	 the frame pointer and offset is a constant.  */
@@ -2293,11 +2123,10 @@ check_mem_read_rtx (rtx *loc, bb_info_t
 			       bb_info->regs_live))
 	    return;
 
-	  if (!store_info->alias_set)
-	    remove = canon_true_dependence (store_info->mem,
-					    GET_MODE (store_info->mem),
-					    store_info->mem_addr,
-					    mem, mem_addr);
+	  remove = canon_true_dependence (store_info->mem,
+					  GET_MODE (store_info->mem),
+					  store_info->mem_addr,
+					  mem, mem_addr);
 
 	  if (remove)
 	    {
@@ -2730,16 +2559,12 @@ dse_step1 (void)
 		  /* Skip the clobbers.  */
 		  while (!store_info->is_set)
 		    store_info = store_info->next;
-		  if (store_info->alias_set && !i_ptr->cannot_delete)
-		    delete_dead_store_insn (i_ptr);
-		  else
-		    if (store_info->group_id >= 0)
-		      {
-			group_info *group
-			  = rtx_group_vec[store_info->group_id];
-			if (group->frame_related && !i_ptr->cannot_delete)
-			  delete_dead_store_insn (i_ptr);
-		      }
+		  if (store_info->group_id >= 0)
+		    {
+		      group_info *group = rtx_group_vec[store_info->group_id];
+		      if (group->frame_related && !i_ptr->cannot_delete)
+			delete_dead_store_insn (i_ptr);
+		    }
 
 		  i_ptr = i_ptr->next_local_store;
 		}
@@ -2868,10 +2693,10 @@ dse_step2_init (void)
 }
 
 
-/* Init the offset tables for the normal case.  */
+/* Init the offset tables.  */
 
 static bool
-dse_step2_nospill (void)
+dse_step2 (void)
 {
   unsigned int i;
   group_info *group;
@@ -2883,9 +2708,6 @@ dse_step2_nospill (void)
       bitmap_iterator bi;
       unsigned int j;
 
-      if (group == clear_alias_group)
-	continue;
-
       memset (group->offset_map_n, 0, sizeof (int) * group->offset_map_size_n);
       memset (group->offset_map_p, 0, sizeof (int) * group->offset_map_size_p);
       bitmap_clear (group->group_kill);
@@ -2945,7 +2767,7 @@ get_bitmap_index (group_info *group_info
    may be NULL. */
 
 static void
-scan_stores_nospill (store_info *store_info, bitmap gen, bitmap kill)
+scan_stores (store_info *store_info, bitmap gen, bitmap kill)
 {
   while (store_info)
     {
@@ -2968,35 +2790,11 @@ scan_stores_nospill (store_info *store_i
 }
 
 
-/* Process the STORE_INFOs into the bitmaps into GEN and KILL.  KILL
-   may be NULL. */
-
-static void
-scan_stores_spill (store_info *store_info, bitmap gen, bitmap kill)
-{
-  while (store_info)
-    {
-      if (store_info->alias_set)
-	{
-	  int index = get_bitmap_index (clear_alias_group,
-					store_info->alias_set);
-	  if (index != 0)
-	    {
-	      bitmap_set_bit (gen, index);
-	      if (kill)
-		bitmap_clear_bit (kill, index);
-	    }
-	}
-      store_info = store_info->next;
-    }
-}
-
-
 /* Process the READ_INFOs into the bitmaps into GEN and KILL.  KILL
    may be NULL.  */
 
 static void
-scan_reads_nospill (insn_info_t insn_info, bitmap gen, bitmap kill)
+scan_reads (insn_info_t insn_info, bitmap gen, bitmap kill)
 {
   read_info_t read_info = insn_info->read_rec;
   int i;
@@ -3086,30 +2884,6 @@ scan_reads_nospill (insn_info_t insn_inf
     }
 }
 
-/* Process the READ_INFOs into the bitmaps into GEN and KILL.  KILL
-   may be NULL.  */
-
-static void
-scan_reads_spill (read_info_t read_info, bitmap gen, bitmap kill)
-{
-  while (read_info)
-    {
-      if (read_info->alias_set)
-	{
-	  int index = get_bitmap_index (clear_alias_group,
-					read_info->alias_set);
-	  if (index != 0)
-	    {
-	      if (kill)
-		bitmap_set_bit (kill, index);
-	      bitmap_clear_bit (gen, index);
-	    }
-	}
-
-      read_info = read_info->next;
-    }
-}
-
 
 /* Return the insn in BB_INFO before the first wild read or if there
    are no wild reads in the block, return the last insn.  */
@@ -3148,16 +2922,12 @@ find_insn_before_first_wild_read (bb_inf
    anything that happens is hidden by the wild read.  */
 
 static void
-dse_step3_scan (bool for_spills, basic_block bb)
+dse_step3_scan (basic_block bb)
 {
   bb_info_t bb_info = bb_table[bb->index];
   insn_info_t insn_info;
 
-  if (for_spills)
-    /* There are no wild reads in the spill case.  */
-    insn_info = bb_info->last_insn;
-  else
-    insn_info = find_insn_before_first_wild_read (bb_info);
+  insn_info = find_insn_before_first_wild_read (bb_info);
 
   /* In the spill case or in the no_spill case if there is no wild
      read in the block, we will need a kill set.  */
@@ -3178,17 +2948,8 @@ dse_step3_scan (bool for_spills, basic_b
 	 this phase.  */
       if (insn_info->insn && INSN_P (insn_info->insn))
 	{
-	  /* Process the read(s) last.  */
-	  if (for_spills)
-	    {
-	      scan_stores_spill (insn_info->store_rec, bb_info->gen, bb_info->kill);
-	      scan_reads_spill (insn_info->read_rec, bb_info->gen, bb_info->kill);
-	    }
-	  else
-	    {
-	      scan_stores_nospill (insn_info->store_rec, bb_info->gen, bb_info->kill);
-	      scan_reads_nospill (insn_info, bb_info->gen, bb_info->kill);
-	    }
+	  scan_stores (insn_info->store_rec, bb_info->gen, bb_info->kill);
+	  scan_reads (insn_info, bb_info->gen, bb_info->kill);
 	}
 
       insn_info = insn_info->prev_insn;
@@ -3243,7 +3004,7 @@ mark_reachable_blocks (sbitmap unreachab
 /* Build the transfer functions for the function.  */
 
 static void
-dse_step3 (bool for_spills)
+dse_step3 ()
 {
   basic_block bb;
   sbitmap unreachable_blocks = sbitmap_alloc (last_basic_block_for_fn (cfun));
@@ -3266,7 +3027,7 @@ dse_step3 (bool for_spills)
       else if (bb->index == EXIT_BLOCK)
 	dse_step3_exit_block_scan (bb_info);
       else
-	dse_step3_scan (for_spills, bb);
+	dse_step3_scan (bb);
       if (EDGE_COUNT (bb->succs) == 0)
 	mark_reachable_blocks (unreachable_blocks, bb);
 
@@ -3467,7 +3228,7 @@ dse_step4 (void)
 
 
 static void
-dse_step5_nospill (void)
+dse_step5 (void)
 {
   basic_block bb;
   FOR_EACH_BB_FN (bb, cfun)
@@ -3502,27 +3263,21 @@ dse_step5_nospill (void)
 	      while (!store_info->is_set)
 		store_info = store_info->next;
 
-	      if (store_info->alias_set)
-		deleted = false;
-	      else
+	      HOST_WIDE_INT i;
+	      group_info *group_info = rtx_group_vec[store_info->group_id];
+
+	      for (i = store_info->begin; i < store_info->end; i++)
 		{
-		  HOST_WIDE_INT i;
-		  group_info *group_info
-		    = rtx_group_vec[store_info->group_id];
+		  int index = get_bitmap_index (group_info, i);
 
-		  for (i = store_info->begin; i < store_info->end; i++)
+		  if (dump_file && (dump_flags & TDF_DETAILS))
+		    fprintf (dump_file, "i = %d, index = %d\n", (int)i, index);
+		  if (index == 0 || !bitmap_bit_p (v, index))
 		    {
-		      int index = get_bitmap_index (group_info, i);
-
 		      if (dump_file && (dump_flags & TDF_DETAILS))
-			fprintf (dump_file, "i = %d, index = %d\n", (int)i, index);
-		      if (index == 0 || !bitmap_bit_p (v, index))
-			{
-			  if (dump_file && (dump_flags & TDF_DETAILS))
-			    fprintf (dump_file, "failing at i = %d\n", (int)i);
-			  deleted = false;
-			  break;
-			}
+			fprintf (dump_file, "failing at i = %d\n", (int)i);
+		      deleted = false;
+		      break;
 		    }
 		}
 	      if (deleted)
@@ -3543,7 +3298,7 @@ dse_step5_nospill (void)
 	      && INSN_P (insn_info->insn)
 	      && (!deleted))
 	    {
-	      scan_stores_nospill (insn_info->store_rec, v, NULL);
+	      scan_stores (insn_info->store_rec, v, NULL);
 	      if (insn_info->wild_read)
 		{
 		  if (dump_file && (dump_flags & TDF_DETAILS))
@@ -3557,7 +3312,7 @@ dse_step5_nospill (void)
 		    fprintf (dump_file, "regular read\n");
                   else if (dump_file && (dump_flags & TDF_DETAILS))
 		    fprintf (dump_file, "non-frame wild read\n");
-		  scan_reads_nospill (insn_info, v, NULL);
+		  scan_reads (insn_info, v, NULL);
 		}
 	    }
 
@@ -3666,23 +3421,23 @@ rest_of_handle_dse (void)
   dse_step0 ();
   dse_step1 ();
   dse_step2_init ();
-  if (dse_step2_nospill ())
+  if (dse_step2 ())
     {
       df_set_flags (DF_LR_RUN_DCE);
       df_analyze ();
       if (dump_file && (dump_flags & TDF_DETAILS))
 	fprintf (dump_file, "doing global processing\n");
-      dse_step3 (false);
+      dse_step3 ();
       dse_step4 ();
-      dse_step5_nospill ();
+      dse_step5 ();
     }
 
   dse_step6 ();
   dse_step7 ();
 
   if (dump_file)
-    fprintf (dump_file, "dse: local deletions = %d, global deletions = %d, spill deletions = %d\n",
-	     locally_deleted, globally_deleted, spill_deleted);
+    fprintf (dump_file, "dse: local deletions = %d, global deletions = %d\n",
+	     locally_deleted, globally_deleted);
 
   /* DSE can eliminate potentially-trapping MEMs.
      Remove any EH edges associated with them.  */

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

[Jakub Jelinek]

On Tue, Apr 05, 2016 at 11:48:29AM +0200, Richard Biener wrote:
> Like this?

Yeah, thanks.

> 2016-04-05  Richard Biener  <rguenther@suse.de>
> 
> 	* dse.c (struct store_info): Remove alias_set member.
> 	(struct read_info_type): Likewise.
> 	(clear_alias_group, clear_alias_mode_table, clear_alias_mode_holder,
> 	spill_deleted, clear_alias_set_lookup): Remove.
> 	(get_group_info): Remove dead base == NULL_RTX case.
> 	(dse_step0): Remove initialization of removed variables.
> 	(delete_dead_store_insn): Reomve alias set dumping.
> 	(free_read_records): Remove alias_set handling.
> 	(canon_address): Remove alias_set_out parameter.
> 	(record_store): Remove spill_alias_set, it's always zero.
> 	(check_mem_read_rtx): Likewise.
> 	(dse_step2): Rename from ...
> 	(dse_step2_nospill): ... this.  Adjust.
> 	(scan_stores): Rename from ...
> 	(scan_stores_nospill): ... this.
> 	(scan_reads): Rename from ...
> 	(scan_reads_nospill): ... this.
> 	(scan_stores_spill, scan_reads_spill): Remove.
> 	(dse_step3_scan): Remove for_spills argument which is always false.
> 	(dse_step3): Likewise.
> 	(dse_step5): Rename from ...
> 	(dse_step5_nospill): ... this.  Remove alias_set handling.
> 	(rest_of_handle_dse): Adjust.

	Jakub