Wilco Dijkstra <wilco.dijks...@arm.com> writes:
> Hi Richard,
>
>> The patch below is what I meant.  It passes bootstrap & regression-test
>> on aarch64-linux-gnu (and so produces the same results for the tests
>> that you changed).  Do you see any problems with this version?
>> If not, I think we should go with it.
>
> Thanks for the detailed example - unfortunately there are issues with it.
> Early expansion means more instructions to deal with in RTL and fewer
> optimizations - it even affects inlining (I see more calls/returns in the
> instruction frequencies).

Do you have an example?  It seems odd that rtl changes after cfgexpand
would affect inlining.

> Worse, this change completely disables rematerialization of FP immediates
> which implies extra spilling. A basic example goes like this:
>
> void g(void);
> double bad_remat (double x)
> {
>   x += 5.347897294;
>   g();
>   x *= 5.347897294;
>   return x;
> }
>
> which with -O2 -fomit-frame-pointer -ffixed-d8 -ffixed-d9 -ffixed-d10 
> -ffixed-d11 -ffixed-d12 -ffixed-d13 -ffixed-d14 now compiles to:
>
>         adrp    x0, .LC0
>         str     x30, [sp, -32]!
>         ldr     d31, [x0, #:lo12:.LC0]
>         str     d15, [sp, 8]
>         fadd    d15, d0, d31
>         str     d31, [sp, 24]
>         bl      g
>         ldr     d31, [sp, 24]
>         fmul    d0, d15, d31
>         ldr     d15, [sp, 8]
>         ldr     x30, [sp], 32
>         ret

Hmm, interesting.  Thanks for the example.

The reason this works with your patch and not mine seems to be a direct
consequence of the fix for PR37273:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=37273

It means that, if we start with:

   (set (reg R) (mem const_pool))
     REG_EQUIV: legitimate_const

then the final costing pass starts out with a significantly negative
memory cost for R.  Assigning memory is taken to be a saving:

  if (set != 0 && REG_P (SET_DEST (set)) && MEM_P (SET_SRC (set))
      && (note = find_reg_note (insn, REG_EQUIV, NULL_RTX)) != NULL_RTX
      && ((MEM_P (XEXP (note, 0))
           && !side_effects_p (SET_SRC (set)))
          || (CONSTANT_P (XEXP (note, 0))
              && targetm.legitimate_constant_p (GET_MODE (SET_DEST (set)),
                                                XEXP (note, 0))
              && REG_N_SETS (REGNO (SET_DEST (set))) == 1))
      && general_operand (SET_SRC (set), GET_MODE (SET_SRC (set)))
      /* LRA does not use equiv with a symbol for PIC code.  */
      && (! ira_use_lra_p || ! pic_offset_table_rtx
          || ! contains_symbol_ref_p (XEXP (note, 0))))

Thus for your patch IRA assigns a call-clobbered register while for
my patch it assumes that memory is cheaper than using caller saves.
If I disable the CONSTANT_P part of the check, to mimic what IRA
does with your patch, then I get the expected output.

The patch for PR37273 predates LRA and so was aimed at reload's
version of caller saves.  AIUI, the problem there was that the
caller saves didn't take advantage of equivalences and so would
always save to the stack.  LRA is smarter than that, so I suspect
the check could be dropped.  We should then get better code in
cases where the rematerialised pseudo is used multiple times
between two calls.

Even so, the code above is clearly assuming that the equivalence
will be replaced by a memory where necessary.  That doesn't happen
due to the following code in lra_constraints:

                /* If it is not a reverse equivalence, we check that a
                   pseudo in rhs of the init insn is not dying in the
                   insn.  Otherwise, the live info at the beginning of
                   the corresponding BB might be wrong after we
                   removed the insn.  When the equiv can be a
                   constant, the right hand side of the init insn can
                   be a pseudo.  */
                || (! reverse_equiv_p (i)
                    && (init_insn_rhs_dead_pseudo_p (i)
                        /* If we reloaded the pseudo in an equivalence
                           init insn, we cannot remove the equiv init
                           insns and the init insns might write into
                           const memory in this case.  */
                        || contains_reloaded_insn_p (i)))

This specifically mentions constants, but the behaviour seems odd
for them.  We don't use init_insns directly to rematerialise the
constant.  We simply replace the pseudo with the constant and reload
it afresh.

So I wouldn't have expected the check above to be necessary for
constants.  In particular, we check elsewhere whether the constant
is legitimate, and things like that.  Adding !CONSTANT_P (x)
to that condition also "fixes" the testcase.

Note that if you change the constant in your example to (say) 7902,
the output with current trunk is similarly bad:

        str     x30, [sp, -32]!
        mov     x0, 244091581366272
        movk    x0, 0x40be, lsl 48
        str     d15, [sp, 8]
        fmov    d15, x0
        fadd    d0, d0, d15
        str     d0, [sp, 24]
        bl      g
        ldr     d0, [sp, 24]
        fmul    d0, d0, d15
        ldr     d15, [sp, 8]
        ldr     x30, [sp], 32
        ret

Even without the secondary reload you mention below, the RA should be
able to reload from the constant pool rather than the stack.

So ISTM that there is an underlying problem that current trunk
happens to work around in some cases.  I don't think what the
move patterns are doing is right on first principles.

> Recent changes have been moving in the opposite direction - keeping
> high-level constructs (like GOT accesses) as a single operation works out
> better for register allocation and allows more optimization.

But I think we should look more into why that is, at least in this case.
(I've tried to start that process above.)  The effect is to hide address
arithmetic from the main allocator (IRA) and only expose it during LRA.
This means that, in high register GPR pressure, LRA has to scavenge and
spill more than it needs to.

Since having insn conditions dependent on can_create_pseudo_p is IMO
wrong in principle, and since the patch wasn't approved in its committed
form, I think we should revert it for GCC 15 and revisit this for GCC 16.

Thanks,
Richard

> So keeping FP immediates as standard move instructions until regalloc
> is best. Supporting MOV/FMOV in regalloc would require another secondary
> reload (and would then allow rematerialization of these constants).
>
> Cheers,
> Wilco

Reply via email to