Patchwork [PR49888,VTA] don't keep VALUEs bound to modified MEMs

login
register
mail settings
Submitter Alexandre Oliva
Date June 20, 2012, 3:52 a.m.
Message ID <orboke1vyr.fsf@livre.localdomain>
Download mbox | patch
Permalink /patch/165872/
State New
Headers show

Comments

Alexandre Oliva - June 20, 2012, 3:52 a.m.
On Jun 16, 2012, "H.J. Lu" <hjl.tools@gmail.com> wrote:

> If I understand it correctly, the new approach fails to handle push
> properly.

It's actually cselib that didn't deal with push properly, so it thinks
incoming stack arguments may be clobbered by them.  But that's not the
whole story, unfortunately.  I still don't have a complete fix for the
problem, but I have some patches that restore nearly all of the passes.

The first one extends RTX alias analysis so that cselib can recognize
that (mem:SI ARGP) and (mem:SI (plus (and (plus ARGP #-4) #-32) #-4))
don't alias.  Before the patch, we'd go for infinite sized objects upon
AND.

The second introduces an entry-point equivalence between ARGP and SP, so
that SP references in push and stack-align sequences can be
canonicalized to ARGP-based.

The third introduces address canonicalization that uses information in
the dataflow variable set in addition to the static cselib table.  This
is the one I'm still working on, because some expressions still fail to
canonicalize to ARGP although they could.

The fourth removes a now-redundant equivalence from the dynamic table;
the required information is always preserved in the static table.

I've regstrapped (and checked results! :-) all of these on
x86_64-linux-gnu and i686-linux-gnu.  It fixes all visible regressions
in x86_64-linux-gnu, and nearly all on i686-linux-gnu.

May I check these in and keep on working to complete the fix, or should
I revert the original patch and come back only with a patchset that
fixes all debug info regressions?
Jakub Jelinek - June 20, 2012, 7:12 a.m.
On Wed, Jun 20, 2012 at 12:52:12AM -0300, Alexandre Oliva wrote:
> On Jun 16, 2012, "H.J. Lu" <hjl.tools@gmail.com> wrote:
> from  Alexandre Oliva  <aoliva@redhat.com>
> 
> 	PR debug/53671
> 	PR debug/49888
> 	* alias.c (memrefs_conflict_p): Improve handling of AND for
> 	alignment.
> from  Alexandre Oliva  <aoliva@redhat.com>
> 
> 	PR debug/53671
> 	PR debug/49888
> 	* var-tracking.c (vt_initialize): Record initial offset between
> 	arg pointer and stack pointer.
> 
> from  Alexandre Oliva  <aoliva@redhat.com>
> 
> 	PR debug/53671
> 	PR debug/49888
> 	* var-tracking.c (vt_get_canonicalize_base): New.
> 	(vt_canonicalize_addr, vt_stack_offset_p): New.
> 	(vt_canon_true_dep): New.
> 	(drop_overlapping_mem_locs): Use vt_canon_true_dep.
> 	(clobber_overlaping_mems): Use vt_canonicalize_addr.
>
> from  Alexandre Oliva  <aoliva@redhat.com>
> 
> 	PR debug/53671
> 	PR debug/49888
> 	* var-tracking.c (vt_init_cfa_base): Drop redundant recording of
> 	CFA base.

Ok, thanks.

	Jakub
Richard Guenther - June 20, 2012, 9 a.m.
On Wed, Jun 20, 2012 at 5:52 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Jun 16, 2012, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>
>> If I understand it correctly, the new approach fails to handle push
>> properly.
>
> It's actually cselib that didn't deal with push properly, so it thinks
> incoming stack arguments may be clobbered by them.  But that's not the
> whole story, unfortunately.  I still don't have a complete fix for the
> problem, but I have some patches that restore nearly all of the passes.
>
> The first one extends RTX alias analysis so that cselib can recognize
> that (mem:SI ARGP) and (mem:SI (plus (and (plus ARGP #-4) #-32) #-4))
> don't alias.  Before the patch, we'd go for infinite sized objects upon
> AND.

Just looking at this patch.  It looks reasonable, but I have a question on
the pre-existing condition

-      if (GET_CODE (y) == AND || ysize < -INTVAL (XEXP (x, 1)))
        xsize = -1;

so if this condition is not true then we simply strip off the AND of X and
do not adjust xsize at all?  Likewise we do not adjust c?  How can that
be conservatively correct?

Thus, I'd rather see

   if (GET_CODE (x) == AND && CONST_INT_P (XEXP (x, 1)))
     {
+      HOST_WIDE_INT sc = INTVAL (XEXP (x, 1));
+      unsigned HOST_WIDE_INT uc = sc;
+      if (xsize > 0 && sc < 0 && -uc == (uc & -uc))
+       {
+         xsize -= sc + 1;
+         c -= sc;
           return memrefs_conflict_p (xsize, canon_rtx (XEXP (x, 0)),
ysize, y, c);
         }
      }

as the sole supported case.  The patch is ok with the above change if it
passes bootstrap & regtest.  I suppose part of the comment that maybe
tries to explain the existing code (but does not match what it does IMHO),

    Assume that references
     besides AND are aligned, so if the size of the other reference is
     at least as large as the alignment, assume no other overlap.  */

should be removed then.

Thanks,
Richard.

>
> The second introduces an entry-point equivalence between ARGP and SP, so
> that SP references in push and stack-align sequences can be
> canonicalized to ARGP-based.
>
> The third introduces address canonicalization that uses information in
> the dataflow variable set in addition to the static cselib table.  This
> is the one I'm still working on, because some expressions still fail to
> canonicalize to ARGP although they could.
>
> The fourth removes a now-redundant equivalence from the dynamic table;
> the required information is always preserved in the static table.
>
> I've regstrapped (and checked results! :-) all of these on
> x86_64-linux-gnu and i686-linux-gnu.  It fixes all visible regressions
> in x86_64-linux-gnu, and nearly all on i686-linux-gnu.
>
> May I check these in and keep on working to complete the fix, or should
> I revert the original patch and come back only with a patchset that
> fixes all debug info regressions?
>
>
>
> --
> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist      Red Hat Brazil Compiler Engineer
>

Patch

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/53671
	PR debug/49888
	* var-tracking.c (vt_init_cfa_base): Drop redundant recording of
	CFA base.

Index: gcc/var-tracking.c
===================================================================
--- gcc/var-tracking.c.orig	2012-06-18 08:44:13.459569497 -0300
+++ gcc/var-tracking.c	2012-06-18 08:55:31.023984364 -0300
@@ -9582,9 +9582,6 @@  vt_init_cfa_base (void)
 				 VOIDmode, get_insns ());
   preserve_value (val);
   cselib_preserve_cfa_base_value (val, REGNO (cfa_base_rtx));
-  var_reg_decl_set (&VTI (ENTRY_BLOCK_PTR)->out, cfa_base_rtx,
-		    VAR_INIT_STATUS_INITIALIZED, dv_from_value (val->val_rtx),
-		    0, NULL_RTX, INSERT);
 }
 
 /* Allocate and initialize the data structures for variable tracking