On Wed, Jun 20, 2012 at 5:52 AM, Alexandre Oliva <aol...@redhat.com> wrote:
> On Jun 16, 2012, "H.J. Lu" <hjl.to...@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
>
