Re: [PATCH v3] Implement new RTL optimizations pass: fold-mem-offsets.

[Manolis Tsamis <manolis.tsamis@vrull.eu>]

I have sent out a new v4 of this
(https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626502.html).

In the new version I both restore the INSN_CODE as mentioned here and
I also call recog when I commit the offset change (because there may
be a change from no offset to some offsets).
I have also removed the driver function as per Vineet's suggestion.

Last thing I have fixed a nasty bug with REG_EQUIV's that resulted in
failing bootstrap on AArch64.

The offending sequence looked like this (in 'simplified RTX'):

(set (reg/f:DI 3 x3)
        (plus:DI (reg/f:DI 2 x2)
            (const_int 8 [0x8])))
(set (reg/f:DI 19 x19)
        (plus:DI (reg/f:DI 3 x3)
            (reg:DI 19 x19)))
...
(set (reg:TI 2 x2)
        (mem:TI (reg/f:DI 0 x0)))
     (expr_list:REG_DEAD (reg/f:DI 0 x0)
        (expr_list:REG_EQUIV (mem:TI (reg/f:DI 19 x19))
            (nil)))
(set (mem:TI (reg/f:DI 19 x19))
        (reg:TI 2 x2))
     (expr_list:REG_DEAD (reg/f:DI 19 x19)
        (expr_list:REG_DEAD (reg:TI 2 x2)
            (nil)))

Were the first instruction (x3 = x2 + 8) was folded in the address
calculation of the last one, resulting in (mem:TI (plus:DI (reg/f:DI
19 x19) (const_int 8 [0x8])), but then the previous REG_EQUIV was
incorrect due to the modified runtime value of x19.
For now I opted to treat REG_EQ/EQUIV notes as uses that we cannot
handle, so if any of them are involved we don't fold offsets. Although
it would may be possible to improve this in the future, I think it's
fine for now and the reduction of folded insns is a small %.
I have tested v4 with a number of benchmarks and large projects on
x64, aarch64 and riscv64 without observing any issues. The x86
testsuite issue still exists as I don't have a satisfactory solution
to it yet.

Any feedback for the changes is appreciated!

Thanks,
Manolis

On Fri, Jul 21, 2023 at 12:59 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 7/20/23 00:18, Vineet Gupta wrote:
> >
> >
> > On 7/18/23 21:31, Jeff Law via Gcc-patches wrote:
> >>>
> >>> In a run with -fno-fold-mem-offsets, the same insn 93 is successfully
> >>> grok'ed by cprop_hardreg,
> >>>
> >>> | (insn 93 337 522 11 (set (mem/c:DF (plus:DI (reg/f:DI 2 sp)
> >>> |                (const_int 8 [0x8])) [4 %sfp+-8 S8 A64])
> >>> |        (const_double:DF 0.0 [0x0.0p+0])) "sff.i":23:11 190
> >>> {*movdf_hardfloat_rv64}
> >>> ^^^^^^^^^^^^^^^
> >>> |     (expr_list:REG_EQUAL (const_double:DF 0.0 [0x0.0p+0])
> >>> |        (nil)))
> >>>
> >>> P.S. I wonder if it is a good idea in general to call recog() post
> >>> reload since the insn could be changed sufficiently to no longer
> >>> match the md patterns. Of course I don't know the answer.
> >> If this ever causes a problem, it's a backend bug.  It's that simple.
> >>
> >> Conceptually it should always be safe to set INSN_CODE to -1 for any
> >> insn.
> >
> > Sure the -1 should be handled, but are you implying that f-mo- will
> > always generate a valid combination and recog() failing is simply a bug
> > in backend and/or f-m-o. If not, the -1 setting can potentially trigger
> > an ICE in future.
> A recog failure after setting INSN_CODE to -1 would always be an
> indicator of a target bug at the point where f-m-o runs.
>
> In that would be generally true as well.  There are some very obscure
> exceptions and those exceptions are for narrow periods of time.
>
>
>
> >
> >>
> >> Odds are for this specific case in the RV backend, we just need a
> >> constraint to store 0.0 into a memory location.  That can actually be
> >> implemented as a store from x0 since 0.0 has the bit pattern 0x0. This
> >> is probably a good thing to expose anyway as an optimization and can
> >> move forward independently of the f-m-o patch.
> >
> > I call dibs on this :-) Seems like an interesting little side project.
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748
> It's yours  :-)
>
> jeff