[PR64164] drop copyrename, integrate into expand

On Aug 19, 2015, Alexandre Oliva <aoliva@redhat.com> wrote:

> On Aug 19, 2015, Alexandre Oliva <aoliva@redhat.com> wrote:
>> I'm having some difficulty getting access to an ia64 box ATM, and for
>> ada bootstraps, a cross won't do, so...  if you still have that build
>> tree around, any chance you could recompile par.o with both stage1 and
>> stage2, with -fdump-rtl-expand-details, and email me the compiler dump
>> files?

> Thanks!

> In the mean time, I have been able to duplicate the problem myself.  As
> you say, it is triggered by -gtoggle.  However, it has nothing
> whatsoever to do with the recent patches I installed.  At most they
> expose some latent problem in the scheduler.

The above was more likely wrong than right.  There may have been a
latent problem in the scheduler indeed, but the patch actually made it
worse, or even introduced it.

The scheduler relies on alias analysis to tell whether a given pair of
insns that read or modify memory should have a dependence set between
them.  It looks both at the RTL proper (including cselib values) and at
the MEM attrs.

The problem was that, for ada/par.o, we computed different dependencies,
and thus different sched priorities, for a pair of insns.  Specifically,
one wrote to a stack spill slot, and another read from a neighbor spill
slot.  Both had MEM_EXPRs pointing at a %sfp decl, and different
offsets.  In the stage3 (-g) compilation, there were debug insns between
them.

They caused additional equivalent expressions to be added to some
values, which in turn caused memrefs_conflict_p to return different
values.

In the stage2 compilation, both VALUEs resolved to PLUSes of two VALUEs,
the first of each resolved to a constant, while the latter of each
resolved to %sfp.  When the second operands of PLUSes match, we recurse
and compare the first operands, resolving both to CONST_INTs and, in
this case, concluding that there's no possible overlap.

In the stage3 compilation, one VALUE resolved to a PLUS of a VALUE and a
CONST_INT, whereas the other resolved to a PLUS of two VALUEs.  Without
canonicalization of VALUE order in PLUSes, it just so happened that the
VALUE that appeared as the second operand in the second PLUS was moved
to the first operand in the first PLUS, and so memrefs_conflict_p
couldn't tell whether or not there was an overlap.

Before the initial pr64164 patch, we had another chance to detect the
non-overlap analyzing the MEM attrs in nonoverlapping_memrefs_p: given
the same MEM_EXPR, but different offsets, we used to conclude there was
no overlap, so this got true_dependence to return the same value in both
compilations.

The pr64164 patch introduced an early exit from nonoverlapping_memrefs_p
when either operand is a gimple_reg, because some of these wouldn't have
a DECL_RTL set, and creating RTL for them at such points would not be
appropriate.  The problem is that the early exit would only return false
if the exprs were different.  If they were the same, we'd conclude an
overlap was possible, even if offsets were enough to tell otherwise.

My thought back then was that such exprs were not addressable anyway, so
we'd always access the entire object, so offsets couldn't possible be
different.  Right?  Well, no!  Think spill slots: %sfp (a gimple_reg
decl) + constant offset!  Same base gimple_reg, non-overlapping memory
addresses!


This patch improves memrefs_conflict_p so as to handle more combinations
of VALUEs in PLUSes: if both incoming addresses are PLUSes, check one
operand of one against the other operand of the other; if one address is
a PLUS and the other isn't, test the other against both operands of the
PLUS.  This causes memrefs_conflict_p to return consistent results for
that given pair of insns in both stage2 and stage3 compilation.

Additionally, it fixes the regression in nonoverlapping_memrefs_p,
adding code to check for non-overlapping offsets when the base expr is
the same (as long as offsets and sizes are known for both MEMs).

Either one would suffice to fix this particular case.  The latter would
fix the regression proper, but the former is sufficiently lightweight
(since comparing pointers is enough) that it's probably worth adding to
get more accurate and consistent results earlier.

I'm bootstrapping this on ia64-linux-gnu.  Ok to install?


fix sched compare regression

From: Alexandre Oliva <aoliva@redhat.com>

for  gcc/ChangeLog

	PR rtl-optimization/64164
	PR rtl-optimization/67227
	* alias.c (memrefs_conflict_p): Handle VALUEs in PLUS better.
	(nonoverlapping_memrefs_p): Test offsets and sizes when given
	identical gimple_reg exprs.
---
 gcc/alias.c |   23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

On Fri, Aug 21, 2015 at 9:46 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Aug 19, 2015, Alexandre Oliva <aoliva@redhat.com> wrote:
>
>> On Aug 19, 2015, Alexandre Oliva <aoliva@redhat.com> wrote:
>>> I'm having some difficulty getting access to an ia64 box ATM, and for
>>> ada bootstraps, a cross won't do, so...  if you still have that build
>>> tree around, any chance you could recompile par.o with both stage1 and
>>> stage2, with -fdump-rtl-expand-details, and email me the compiler dump
>>> files?
>
>> Thanks!
>
>> In the mean time, I have been able to duplicate the problem myself.  As
>> you say, it is triggered by -gtoggle.  However, it has nothing
>> whatsoever to do with the recent patches I installed.  At most they
>> expose some latent problem in the scheduler.
>
> The above was more likely wrong than right.  There may have been a
> latent problem in the scheduler indeed, but the patch actually made it
> worse, or even introduced it.
>
> The scheduler relies on alias analysis to tell whether a given pair of
> insns that read or modify memory should have a dependence set between
> them.  It looks both at the RTL proper (including cselib values) and at
> the MEM attrs.
>
> The problem was that, for ada/par.o, we computed different dependencies,
> and thus different sched priorities, for a pair of insns.  Specifically,
> one wrote to a stack spill slot, and another read from a neighbor spill
> slot.  Both had MEM_EXPRs pointing at a %sfp decl, and different
> offsets.  In the stage3 (-g) compilation, there were debug insns between
> them.
>
> They caused additional equivalent expressions to be added to some
> values, which in turn caused memrefs_conflict_p to return different
> values.
>
> In the stage2 compilation, both VALUEs resolved to PLUSes of two VALUEs,
> the first of each resolved to a constant, while the latter of each
> resolved to %sfp.  When the second operands of PLUSes match, we recurse
> and compare the first operands, resolving both to CONST_INTs and, in
> this case, concluding that there's no possible overlap.
>
> In the stage3 compilation, one VALUE resolved to a PLUS of a VALUE and a
> CONST_INT, whereas the other resolved to a PLUS of two VALUEs.  Without
> canonicalization of VALUE order in PLUSes, it just so happened that the
> VALUE that appeared as the second operand in the second PLUS was moved
> to the first operand in the first PLUS, and so memrefs_conflict_p
> couldn't tell whether or not there was an overlap.
>
> Before the initial pr64164 patch, we had another chance to detect the
> non-overlap analyzing the MEM attrs in nonoverlapping_memrefs_p: given
> the same MEM_EXPR, but different offsets, we used to conclude there was
> no overlap, so this got true_dependence to return the same value in both
> compilations.
>
> The pr64164 patch introduced an early exit from nonoverlapping_memrefs_p
> when either operand is a gimple_reg, because some of these wouldn't have
> a DECL_RTL set, and creating RTL for them at such points would not be
> appropriate.  The problem is that the early exit would only return false
> if the exprs were different.  If they were the same, we'd conclude an
> overlap was possible, even if offsets were enough to tell otherwise.
>
> My thought back then was that such exprs were not addressable anyway, so
> we'd always access the entire object, so offsets couldn't possible be
> different.  Right?  Well, no!  Think spill slots: %sfp (a gimple_reg
> decl) + constant offset!  Same base gimple_reg, non-overlapping memory
> addresses!
>
>
> This patch improves memrefs_conflict_p so as to handle more combinations
> of VALUEs in PLUSes: if both incoming addresses are PLUSes, check one
> operand of one against the other operand of the other; if one address is
> a PLUS and the other isn't, test the other against both operands of the
> PLUS.  This causes memrefs_conflict_p to return consistent results for
> that given pair of insns in both stage2 and stage3 compilation.
>
> Additionally, it fixes the regression in nonoverlapping_memrefs_p,
> adding code to check for non-overlapping offsets when the base expr is
> the same (as long as offsets and sizes are known for both MEMs).
>
> Either one would suffice to fix this particular case.  The latter would
> fix the regression proper, but the former is sufficiently lightweight
> (since comparing pointers is enough) that it's probably worth adding to
> get more accurate and consistent results earlier.
>
> I'm bootstrapping this on ia64-linux-gnu.  Ok to install?

Looks ok to me.

Thanks,
Richard.

>
> fix sched compare regression
>
> From: Alexandre Oliva <aoliva@redhat.com>
>
> for  gcc/ChangeLog
>
>         PR rtl-optimization/64164
>         PR rtl-optimization/67227
>         * alias.c (memrefs_conflict_p): Handle VALUEs in PLUS better.
>         (nonoverlapping_memrefs_p): Test offsets and sizes when given
>         identical gimple_reg exprs.
> ---
>  gcc/alias.c |   23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/alias.c b/gcc/alias.c
> index 4681e3f..f12d9d1 100644
> --- a/gcc/alias.c
> +++ b/gcc/alias.c
> @@ -2228,6 +2228,13 @@ memrefs_conflict_p (int xsize, rtx x, int ysize, rtx y, HOST_WIDE_INT c)
>        rtx x0 = XEXP (x, 0);
>        rtx x1 = XEXP (x, 1);
>
> +      /* However, VALUEs might end up in different positions even in
> +        canonical PLUSes.  Comparing their addresses is enough.  */
> +      if (x0 == y)
> +       return memrefs_conflict_p (xsize, x1, ysize, const0_rtx, c);
> +      else if (x1 == y)
> +       return memrefs_conflict_p (xsize, x0, ysize, const0_rtx, c);
> +
>        if (GET_CODE (y) == PLUS)
>         {
>           /* The fact that Y is canonicalized means that this
> @@ -2235,6 +2242,11 @@ memrefs_conflict_p (int xsize, rtx x, int ysize, rtx y, HOST_WIDE_INT c)
>           rtx y0 = XEXP (y, 0);
>           rtx y1 = XEXP (y, 1);
>
> +         if (x0 == y1)
> +           return memrefs_conflict_p (xsize, x1, ysize, y0, c);
> +         if (x1 == y0)
> +           return memrefs_conflict_p (xsize, x0, ysize, y1, c);
> +
>           if (rtx_equal_for_memref_p (x1, y1))
>             return memrefs_conflict_p (xsize, x0, ysize, y0, c);
>           if (rtx_equal_for_memref_p (x0, y0))
> @@ -2263,6 +2275,11 @@ memrefs_conflict_p (int xsize, rtx x, int ysize, rtx y, HOST_WIDE_INT c)
>        rtx y0 = XEXP (y, 0);
>        rtx y1 = XEXP (y, 1);
>
> +      if (x == y0)
> +       return memrefs_conflict_p (xsize, const0_rtx, ysize, y1, c);
> +      if (x == y1)
> +       return memrefs_conflict_p (xsize, const0_rtx, ysize, y0, c);
> +
>        if (CONST_INT_P (y1))
>         return memrefs_conflict_p (xsize, x, ysize, y0, c + INTVAL (y1));
>        else
> @@ -2518,7 +2535,11 @@ nonoverlapping_memrefs_p (const_rtx x, const_rtx y, bool loop_invariant)
>       able to do anything about them since no SSA information will have
>       remained to guide it.  */
>    if (is_gimple_reg (exprx) || is_gimple_reg (expry))
> -    return exprx != expry;
> +    return exprx != expry
> +      || (moffsetx_known_p && moffsety_known_p
> +         && MEM_SIZE_KNOWN_P (x) && MEM_SIZE_KNOWN_P (y)
> +         && !offset_overlap_p (moffsety - moffsetx,
> +                               MEM_SIZE (x), MEM_SIZE (y)));
>
>    /* With invalid code we can end up storing into the constant pool.
>       Bail out to avoid ICEing when creating RTL for this.
>
>
> --
> 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 Brasil GNU Toolchain Engineer

Alexandre Oliva <aoliva@redhat.com> writes:

> 	PR rtl-optimization/64164
> 	PR rtl-optimization/67227
> 	* alias.c (memrefs_conflict_p): Handle VALUEs in PLUS better.
> 	(nonoverlapping_memrefs_p): Test offsets and sizes when given
> 	identical gimple_reg exprs.

I can confirm that this fixes the bootstrap.

Thanks, Andreas.