Re: [PR64164] drop copyrename, integrate into expand

[Richard Biener <richard dot guenther at gmail dot com>]

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