RISC-V: Folding memory for FP + constant case

RISC-V: Folding memory for FP + constant case

[Jivan Hakobyan]

[-- Attachment #1.1: Type: text/plain, Size: 888 bytes --]

Accessing local arrays element turned into load form (fp + (index << C1)) +
C2 address.
In the case when access is in the loop we got loop invariant computation.
For some reason, moving out that part cannot be done in
loop-invariant passes.
But we can handle that in target-specific hook (legitimize_address).
That provides an opportunity to rewrite memory access more suitable for the
target architecture.

This patch solves the mentioned case by rewriting mentioned case to ((fp +
C2) + (index << C1))
I have evaluated it on SPEC2017 and got an improvement on leela (over 7b
instructions,
.39% of the dynamic count) and dwarfs the regression for gcc (14m
instructions, .0012%
of the dynamic count).


gcc/ChangeLog:
        * config/riscv/riscv.cc (riscv_legitimize_address): Handle folding.
        (mem_shadd_or_shadd_rtx_p): New predicate.


-- 
With the best regards
Jivan Hakobyan

[-- Attachment #2: legitimize_address.diff --]
[-- Type: text/x-patch, Size: 2530 bytes --]

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index e4dc8115e696ed44affe6ee8b51d635fe0eaaa33..2a7e464b855ec45f1fce4daec36d84842f3f3ea4 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -1754,6 +1754,22 @@ riscv_shorten_lw_offset (rtx base, HOST_WIDE_INT offset)
   return addr;
 }
 
+/* Helper for riscv_legitimize_address. Given X, return true if it
+   is a left shift by 1, 2 or 3 positions or a multiply by 2, 4 or 8.
+
+   This respectively represent canonical shift-add rtxs or scaled
+   memory addresses.  */
+static bool
+mem_shadd_or_shadd_rtx_p (rtx x)
+{
+  return ((GET_CODE (x) == ASHIFT
+           || GET_CODE (x) == MULT)
+          && GET_CODE (XEXP (x, 1)) == CONST_INT
+          && ((GET_CODE (x) == ASHIFT && IN_RANGE (INTVAL (XEXP (x, 1)), 1, 3))
+              || (GET_CODE (x) == MULT
+                  && IN_RANGE (exact_log2 (INTVAL (XEXP (x, 1))), 1, 3))));
+}
+
 /* This function is used to implement LEGITIMIZE_ADDRESS.  If X can
    be legitimized in a way that the generic machinery might not expect,
    return a new address, otherwise return NULL.  MODE is the mode of
@@ -1779,6 +1795,33 @@ riscv_legitimize_address (rtx x, rtx oldx ATTRIBUTE_UNUSED,
       rtx base = XEXP (x, 0);
       HOST_WIDE_INT offset = INTVAL (XEXP (x, 1));
 
+      /* Handle (plus (plus (mult (a) (mem_shadd_constant)) (fp)) (C)) case.  */
+      if (GET_CODE (base) == PLUS && mem_shadd_or_shadd_rtx_p (XEXP (base, 0))
+          && SMALL_OPERAND (offset))
+        {
+
+          rtx index = XEXP (base, 0);
+          rtx fp = XEXP (base, 1);
+          if (REGNO (fp) == VIRTUAL_STACK_VARS_REGNUM)
+            {
+
+              /* If we were given a MULT, we must fix the constant
+                 as we're going to create the ASHIFT form.  */
+              int shift_val = INTVAL (XEXP (index, 1));
+              if (GET_CODE (index) == MULT)
+                shift_val = exact_log2 (shift_val);
+
+              rtx reg1 = gen_reg_rtx (Pmode);
+              rtx reg2 = gen_reg_rtx (Pmode);
+              rtx reg3 = gen_reg_rtx (Pmode);
+              riscv_emit_binary (PLUS, reg1, fp, GEN_INT (offset));
+              riscv_emit_binary (ASHIFT, reg2, XEXP (index, 0), GEN_INT (shift_val));
+              riscv_emit_binary (PLUS, reg3, reg2, reg1);
+
+              return reg3;
+            }
+        }
+
       if (!riscv_valid_base_register_p (base, mode, false))
 	base = copy_to_mode_reg (Pmode, base);
       if (optimize_function_for_size_p (cfun)

Re: RISC-V: Folding memory for FP + constant case

[Jeff Law]

On 7/12/23 14:59, Jivan Hakobyan via Gcc-patches wrote:
> Accessing local arrays element turned into load form (fp + (index <<
> C1)) + C2 address. In the case when access is in the loop we got loop
> invariant computation. For some reason, moving out that part cannot
> be done in loop-invariant passes. But we can handle that in
> target-specific hook (legitimize_address). That provides an
> opportunity to rewrite memory access more suitable for the target
> architecture.
> 
> This patch solves the mentioned case by rewriting mentioned case to
> ((fp + C2) + (index << C1)) I have evaluated it on SPEC2017 and got
> an improvement on leela (over 7b instructions, .39% of the dynamic
> count) and dwarfs the regression for gcc (14m instructions, .0012% of
> the dynamic count).
> 
> 
> gcc/ChangeLog: * config/riscv/riscv.cc (riscv_legitimize_address):
> Handle folding. (mem_shadd_or_shadd_rtx_p): New predicate.
So I still need to give the new version a review.  But a high level 
question -- did you re-run the benchmarks with this version to verify 
that we still saw the same nice improvement in leela?

The reason I ask is when I use this on Ventana's internal tree I don't 
see any notable differences in the dynamic instruction counts.  And 
probably the most critical difference between the upstream tree and 
Ventana's tree in this space is Ventana's internal tree has an earlier 
version of the fold-mem-offsets work from Manolis.

It may ultimately be the case that this work and Manolis's f-m-o patch 
have a lot of overlap in terms of their final effect on code generation. 
  Manolis's pass runs much later (after register allocation), so it's 
not going to address the loop-invariant-code-motion issue that 
originally got us looking into this space.  But his pass is generic 
enough that it helps other targets.  So we may ultimately want both.

Anyway, just wanted to verify if this variant is still showing the nice 
improvement on leela that the prior version did.

Jeff

ps.  I know you're on PTO.  No rush on responding -- enjoy the time off.

Re: RISC-V: Folding memory for FP + constant case

[Jivan Hakobyan]

[-- Attachment #1: Type: text/plain, Size: 3367 bytes --]

Hi.

I re-run the benchmarks and hopefully got the same profit.
I also compared the leela's code and figured out the reason.

Actually, my and Manolis's patches do the same thing. The difference is
only execution order.
Because of f-m-o held after the register allocation it cannot eliminate
redundant move 'sp' to another register.

Here is an example.

int core_bench_state(int *ptr) {
>    int final_counts[100] = {0};

   while (*ptr) {
>      int id = foo();
>      final_counts[id]++;
>      ptr++;
>    }

   return final_counts[0];
> }


For this loop, the f-m-o pass generates the following.

.L3:

call foo
* mv a5,sp*
sh2add a0,a0,a5
lw a5,0(a0)
lw a4,4(s0)
addi s0,s0,4
addiw a5,a5,1
sw a5,0(a0)
bne a4,zero,.L3


Here '*mv a5, sp*' instruction is redundant.
Leela's FastState::try_move() function has a loop that iterates over 1.3 B
times and contains 5 memory folding cases (5 redundant moves).

Besides that, I have checked the build failure on x264_r. It is already
fixed on the third version.

On Sat, Jul 15, 2023 at 10:16 AM Jeff Law <jeffreyalaw@gmail.com> wrote:

>
>
> On 7/12/23 14:59, Jivan Hakobyan via Gcc-patches wrote:
> > Accessing local arrays element turned into load form (fp + (index <<
> > C1)) + C2 address. In the case when access is in the loop we got loop
> > invariant computation. For some reason, moving out that part cannot
> > be done in loop-invariant passes. But we can handle that in
> > target-specific hook (legitimize_address). That provides an
> > opportunity to rewrite memory access more suitable for the target
> > architecture.
> >
> > This patch solves the mentioned case by rewriting mentioned case to
> > ((fp + C2) + (index << C1)) I have evaluated it on SPEC2017 and got
> > an improvement on leela (over 7b instructions, .39% of the dynamic
> > count) and dwarfs the regression for gcc (14m instructions, .0012% of
> > the dynamic count).
> >
> >
> > gcc/ChangeLog: * config/riscv/riscv.cc (riscv_legitimize_address):
> > Handle folding. (mem_shadd_or_shadd_rtx_p): New predicate.
> So I still need to give the new version a review.  But a high level
> question -- did you re-run the benchmarks with this version to verify
> that we still saw the same nice improvement in leela?
>
> The reason I ask is when I use this on Ventana's internal tree I don't
> see any notable differences in the dynamic instruction counts.  And
> probably the most critical difference between the upstream tree and
> Ventana's tree in this space is Ventana's internal tree has an earlier
> version of the fold-mem-offsets work from Manolis.
>
> It may ultimately be the case that this work and Manolis's f-m-o patch
> have a lot of overlap in terms of their final effect on code generation.
>   Manolis's pass runs much later (after register allocation), so it's
> not going to address the loop-invariant-code-motion issue that
> originally got us looking into this space.  But his pass is generic
> enough that it helps other targets.  So we may ultimately want both.
>
> Anyway, just wanted to verify if this variant is still showing the nice
> improvement on leela that the prior version did.
>
> Jeff
>
> ps.  I know you're on PTO.  No rush on responding -- enjoy the time off.
>
>

-- 
With the best regards
Jivan Hakobyan

Re: RISC-V: Folding memory for FP + constant case

[Jeff Law]

On 7/25/23 05:24, Jivan Hakobyan wrote:
> Hi.
> 
> I re-run the benchmarks and hopefully got the same profit.
> I also compared the leela's code and figured out the reason.
> 
> Actually, my and Manolis's patches do the same thing. The difference is 
> only execution order.
But shouldn't your patch also allow for for at the last the potential to 
pull the fp+offset computation out of a loop?  I'm pretty sure Manolis's 
patch can't do that.

> Because of f-m-o held after the register allocation it cannot eliminate 
> redundant move 'sp' to another register.
Actually that's supposed to be handled by a different patch that should 
already be upstream.  Specifically;

> commit 6a2e8dcbbd4bab374b27abea375bf7a921047800
> Author: Manolis Tsamis <manolis.tsamis@vrull.eu>
> Date:   Thu May 25 13:44:41 2023 +0200
> 
>     cprop_hardreg: Enable propagation of the stack pointer if possible
>     
>     Propagation of the stack pointer in cprop_hardreg is currenty
>     forbidden in all cases, due to maybe_mode_change returning NULL.
>     Relax this restriction and allow propagation when no mode change is
>     requested.
>     
>     gcc/ChangeLog:
>     
>             * regcprop.cc (maybe_mode_change): Enable stack pointer
>             propagation.
I think there were a couple-follow-ups.  But that's the key change that 
should allow propagation of copies from the stack pointer and thus 
eliminate the mov gpr,sp instructions.  If that's not happening, then 
it's worth investigating why.

> 
> Besides that, I have checked the build failure on x264_r. It is already 
> fixed on the third version.
Yea, this was a problem with re-recognition.  I think it was fixed by:

> commit ecfa870ff29d979bd2c3d411643b551f2b6915b0
> Author: Vineet Gupta <vineetg@rivosinc.com>
> Date:   Thu Jul 20 11:15:37 2023 -0700
> 
>     RISC-V: optim const DF +0.0 store to mem [PR/110748]
>     
>     Fixes: ef85d150b5963 ("RISC-V: Enable TARGET_SUPPORTS_WIDE_INT")
>     
>     DF +0.0 is bitwise all zeros so int x0 store to mem can be used to optimize it.
[ ... ]


So I think the big question WRT your patch is does it still help the 
case where we weren't pulling the fp+offset computation out of a loop.

Jeff

Re: RISC-V: Folding memory for FP + constant case

[Vineet Gupta]

[-- Attachment #1: Type: text/plain, Size: 2508 bytes --]



On 7/25/23 20:31, Jeff Law via Gcc-patches wrote:
>
>
> On 7/25/23 05:24, Jivan Hakobyan wrote:
>> Hi.
>>
>> I re-run the benchmarks and hopefully got the same profit.
>> I also compared the leela's code and figured out the reason.
>>
>> Actually, my and Manolis's patches do the same thing. The difference 
>> is only execution order.
> But shouldn't your patch also allow for for at the last the potential 
> to pull the fp+offset computation out of a loop?  I'm pretty sure 
> Manolis's patch can't do that.
>
>> Because of f-m-o held after the register allocation it cannot 
>> eliminate redundant move 'sp' to another register.
> Actually that's supposed to be handled by a different patch that 
> should already be upstream.  Specifically;
>
>> commit 6a2e8dcbbd4bab374b27abea375bf7a921047800
>> Author: Manolis Tsamis <manolis.tsamis@vrull.eu>
>> Date:   Thu May 25 13:44:41 2023 +0200
>>
>>     cprop_hardreg: Enable propagation of the stack pointer if possible
>>         Propagation of the stack pointer in cprop_hardreg is currenty
>>     forbidden in all cases, due to maybe_mode_change returning NULL.
>>     Relax this restriction and allow propagation when no mode change is
>>     requested.
>>         gcc/ChangeLog:
>>                 * regcprop.cc (maybe_mode_change): Enable stack pointer
>>             propagation.
> I think there were a couple-follow-ups.  But that's the key change 
> that should allow propagation of copies from the stack pointer and 
> thus eliminate the mov gpr,sp instructions.  If that's not happening, 
> then it's worth investigating why.
>
>>
>> Besides that, I have checked the build failure on x264_r. It is 
>> already fixed on the third version.
> Yea, this was a problem with re-recognition.  I think it was fixed by:
>
>> commit ecfa870ff29d979bd2c3d411643b551f2b6915b0
>> Author: Vineet Gupta <vineetg@rivosinc.com>
>> Date:   Thu Jul 20 11:15:37 2023 -0700
>>
>>     RISC-V: optim const DF +0.0 store to mem [PR/110748]
>>         Fixes: ef85d150b5963 ("RISC-V: Enable TARGET_SUPPORTS_WIDE_INT")
>>         DF +0.0 is bitwise all zeros so int x0 store to mem can be 
>> used to optimize it.
> [ ... ]
>
>
> So I think the big question WRT your patch is does it still help the 
> case where we weren't pulling the fp+offset computation out of a loop.

I have some numbers for f-m-o v3 vs this. Attached here (vs. inline to 
avoid the Thunderbird mangling the test formatting)

[-- Attachment #2: fmo-vs-fold-fp --]
[-- Type: text/plain, Size: 2698 bytes --]

benchmark	workload #       upstream        upstream +              upstream +
                                 g54e54f77c1        f-m-o               fold-fp-off

500.perlbench_r	0		1217932817476	1217884553366	0.004%	1217928953834	0.000%
		1		743757241201	743655528133	0.014%	743695820426	0.008%
		2		703455646090	703423559298	0.005%	703455296251	0.000%
502.gcc_r	0		195004369104	194973478945	0.016%	194984188400	0.010%
		1		232719938778	232688491113	0.014%	232692379085	0.012%
		2		223443280459	223413616368	0.013%	223424151848	0.009%
		3		186233704624	186206516421	0.015%	186231137616	0.001%
		4		287406394232	287378870279	0.010%	287403707466	0.001%
503.bwaves_r	0		316194043679	316194043679	0.000%	316194043662	0.000%
		1		499293490380	499293490380	0.000%	499293490363	0.000%
		2		389365401615	389365401615	0.000%	389365401598	0.000%
		3		473514310679	473514310679	0.000%	473514310662	0.000%
505.mcf_r	0		689258694902	689254740344	0.001%	689258694887	0.000%
507.cactuBSSN_r	0		3966612364613	3966498234698	0.003%	3966612365068	0.000%
508.namd_r	0		1903766272166	1903766271701	0.000%	1903765987301	0.000%
510.parest_r	0		3512678127316	3512676752062	0.000%	3512677505662	0.008%
511.povray_r	0		3036725558618	3036722265149	0.000%	3036725556997	0.000%
519.lbm_r	0		1134454304533	1134454304533	0.000%	1134454304518	0.000%
520.omnetpp_r	0		1001937885126	1001937884542	0.000%	1001937883931	0.000%
521.wrf_r	0		3959642601629	3959541912013	0.003%	3959642615086	0.000%
523.xalancbmk_r	0		1065004269065	1064981413043	0.002%	1065004132070	0.000%
525.x264_r	0		496492857533	496459367582	0.007%	496477988435	0.003%
		1		1891248078083	1891222197535	0.001%	1890990911614	0.014%
		2		1815609267498	1815561397105	0.003%	1815341248007	0.015%
526.blender_r	0		1672203767444	1671549923427	0.039%	1672224626743  -0.001%
527.cam4_r	0		2326424925038	2320567166886	0.252%	2326333566227	0.004% <-
531.deepsjeng_r	0		1668993359340	1662816376544	0.370%	1668993353038	0.000% <-
538.imagick_r	0		3260965672876	3260965672712	0.000%	3260965672777	0.000%
541.leela_r	0		2034139863891	2034101807341	0.002%	2026647843672	0.368%    <--
544.nab_r	0		1566465507272	1565420628706	0.067%	1566465379674	0.000%
548.exchange2_r	0		2228112071994	2228109962469	0.000%	2228114278251	0.000%
549.fotonik3d_r	0		2255238867247	2255238867246	0.000%	2255238865924	0.000%
554.roms_r	0		2653150555486	2651884870455	0.048%	2653150554877	0.000%
557.xz_r	0		367892301169	367892301167	0.000%	367892301154	0.000%
		1		979549393200	979549393198	0.000%	979549393185	0.000%
		2		525066235331	525066235329	0.000%	525066235316	0.000%
997.specrand_fr	0		453112389	453112389	0.000%	453112374	0.000%
999.specrand_ir	0		453112389	453112389	0.000%	453112374	0.000%

Re: RISC-V: Folding memory for FP + constant case

[Jivan Hakobyan]

[-- Attachment #1: Type: text/plain, Size: 2844 bytes --]

Thank you for your effort.
I had evaluated only in intrate tests.
I am glad to see the same result on Leela.

On Tue, Aug 1, 2023 at 11:14 PM Vineet Gupta <vineetg@rivosinc.com> wrote:

>
>
> On 7/25/23 20:31, Jeff Law via Gcc-patches wrote:
> >
> >
> > On 7/25/23 05:24, Jivan Hakobyan wrote:
> >> Hi.
> >>
> >> I re-run the benchmarks and hopefully got the same profit.
> >> I also compared the leela's code and figured out the reason.
> >>
> >> Actually, my and Manolis's patches do the same thing. The difference
> >> is only execution order.
> > But shouldn't your patch also allow for for at the last the potential
> > to pull the fp+offset computation out of a loop?  I'm pretty sure
> > Manolis's patch can't do that.
> >
> >> Because of f-m-o held after the register allocation it cannot
> >> eliminate redundant move 'sp' to another register.
> > Actually that's supposed to be handled by a different patch that
> > should already be upstream.  Specifically;
> >
> >> commit 6a2e8dcbbd4bab374b27abea375bf7a921047800
> >> Author: Manolis Tsamis <manolis.tsamis@vrull.eu>
> >> Date:   Thu May 25 13:44:41 2023 +0200
> >>
> >>     cprop_hardreg: Enable propagation of the stack pointer if possible
> >>         Propagation of the stack pointer in cprop_hardreg is currenty
> >>     forbidden in all cases, due to maybe_mode_change returning NULL.
> >>     Relax this restriction and allow propagation when no mode change is
> >>     requested.
> >>         gcc/ChangeLog:
> >>                 * regcprop.cc (maybe_mode_change): Enable stack pointer
> >>             propagation.
> > I think there were a couple-follow-ups.  But that's the key change
> > that should allow propagation of copies from the stack pointer and
> > thus eliminate the mov gpr,sp instructions.  If that's not happening,
> > then it's worth investigating why.
> >
> >>
> >> Besides that, I have checked the build failure on x264_r. It is
> >> already fixed on the third version.
> > Yea, this was a problem with re-recognition.  I think it was fixed by:
> >
> >> commit ecfa870ff29d979bd2c3d411643b551f2b6915b0
> >> Author: Vineet Gupta <vineetg@rivosinc.com>
> >> Date:   Thu Jul 20 11:15:37 2023 -0700
> >>
> >>     RISC-V: optim const DF +0.0 store to mem [PR/110748]
> >>         Fixes: ef85d150b5963 ("RISC-V: Enable TARGET_SUPPORTS_WIDE_INT")
> >>         DF +0.0 is bitwise all zeros so int x0 store to mem can be
> >> used to optimize it.
> > [ ... ]
> >
> >
> > So I think the big question WRT your patch is does it still help the
> > case where we weren't pulling the fp+offset computation out of a loop.
>
> I have some numbers for f-m-o v3 vs this. Attached here (vs. inline to
> avoid the Thunderbird mangling the test formatting)
>


-- 
With the best regards
Jivan Hakobyan

Re: RISC-V: Folding memory for FP + constant case

[Jeff Law]

On 8/1/23 13:14, Vineet Gupta wrote:

> 
> I have some numbers for f-m-o v3 vs this. Attached here (vs. inline to 
> avoid the Thunderbird mangling the test formatting)
Thanks.  Of particular importance is the leela change.  My recollection 
was that the f-m-o work also picked up that case.  But if my memory is 
faulty (always a possibility), then that shows a clear case where 
Jivan's work picks up a case not handled by Manolis's work.

And on the other direction we can see that deepsjeng isn't helped by 
Jivan's work, but is helped by Manolis's new pass.

I'd always hoped/expected we'd have cases where one patch clearly helped 
over the other.  While the .25% to .37% improvements for the three most 
impacted benchmarks doesn't move the needle much across the whole suite 
they do add up over time.

Jeff

Re: RISC-V: Folding memory for FP + constant case

[Philipp Tomsich]

+Manolis Tsamis

On Tue, 1 Aug 2023 at 23:56, Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 8/1/23 13:14, Vineet Gupta wrote:
>
> >
> > I have some numbers for f-m-o v3 vs this. Attached here (vs. inline to
> > avoid the Thunderbird mangling the test formatting)
> Thanks.  Of particular importance is the leela change.  My recollection
> was that the f-m-o work also picked up that case.  But if my memory is
> faulty (always a possibility), then that shows a clear case where
> Jivan's work picks up a case not handled by Manolis's work.

f-m-o originally targeted (and benefited) the leela-case.  I wonder if
other optimizations/changes over the last year interfere with this and
what needs to be changed to accomodate this... looks like we need to
revisit against trunk.

Philipp.

> And on the other direction we can see that deepsjeng isn't helped by
> Jivan's work, but is helped by Manolis's new pass.
>
> I'd always hoped/expected we'd have cases where one patch clearly helped
> over the other.  While the .25% to .37% improvements for the three most
> impacted benchmarks doesn't move the needle much across the whole suite
> they do add up over time.
>
> Jeff

Re: RISC-V: Folding memory for FP + constant case

[Vineet Gupta]

On 8/1/23 15:07, Philipp Tomsich wrote:
> +Manolis Tsamis
>
> On Tue, 1 Aug 2023 at 23:56, Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>>
>> On 8/1/23 13:14, Vineet Gupta wrote:
>>
>>> I have some numbers for f-m-o v3 vs this. Attached here (vs. inline to
>>> avoid the Thunderbird mangling the test formatting)
>> Thanks.  Of particular importance is the leela change.  My recollection
>> was that the f-m-o work also picked up that case.  But if my memory is
>> faulty (always a possibility), then that shows a clear case where
>> Jivan's work picks up a case not handled by Manolis's work.
> f-m-o originally targeted (and benefited) the leela-case.  I wonder if
> other optimizations/changes over the last year interfere with this and
> what needs to be changed to accomodate this... looks like we need to
> revisit against trunk.
>
> Philipp.
>
>> And on the other direction we can see that deepsjeng isn't helped by
>> Jivan's work, but is helped by Manolis's new pass.
>>
>> I'd always hoped/expected we'd have cases where one patch clearly helped
>> over the other.  While the .25% to .37% improvements for the three most
>> impacted benchmarks doesn't move the needle much across the whole suite
>> they do add up over time.
>>
>> Jeff

I took a quick look at Leela, the significant difference is from 
additional insns with SP not getting propagated.

e.g.

    231b6:    mv    a4,sp
    231b8:    sh2add    a5,a5,a4

vs.

    1e824:    sh2add    a5,a5,sp

There are 5 such instances which more or less make up for the delta.

-Vineet

Re: RISC-V: Folding memory for FP + constant case

[Philipp Tomsich]

Very helpful! Looks as if regprop for stack_pointer is now either too
conservative — or one of our patches is missing in everyone's test
setup; we'll take a closer look.

On Wed, 2 Aug 2023 at 01:03, Vineet Gupta <vineetg@rivosinc.com> wrote:
>
>
>
> On 8/1/23 15:07, Philipp Tomsich wrote:
> > +Manolis Tsamis
> >
> > On Tue, 1 Aug 2023 at 23:56, Jeff Law via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >>
> >> On 8/1/23 13:14, Vineet Gupta wrote:
> >>
> >>> I have some numbers for f-m-o v3 vs this. Attached here (vs. inline to
> >>> avoid the Thunderbird mangling the test formatting)
> >> Thanks.  Of particular importance is the leela change.  My recollection
> >> was that the f-m-o work also picked up that case.  But if my memory is
> >> faulty (always a possibility), then that shows a clear case where
> >> Jivan's work picks up a case not handled by Manolis's work.
> > f-m-o originally targeted (and benefited) the leela-case.  I wonder if
> > other optimizations/changes over the last year interfere with this and
> > what needs to be changed to accomodate this... looks like we need to
> > revisit against trunk.
> >
> > Philipp.
> >
> >> And on the other direction we can see that deepsjeng isn't helped by
> >> Jivan's work, but is helped by Manolis's new pass.
> >>
> >> I'd always hoped/expected we'd have cases where one patch clearly helped
> >> over the other.  While the .25% to .37% improvements for the three most
> >> impacted benchmarks doesn't move the needle much across the whole suite
> >> they do add up over time.
> >>
> >> Jeff
>
> I took a quick look at Leela, the significant difference is from
> additional insns with SP not getting propagated.
>
> e.g.
>
>     231b6:    mv    a4,sp
>     231b8:    sh2add    a5,a5,a4
>
> vs.
>
>     1e824:    sh2add    a5,a5,sp
>
> There are 5 such instances which more or less make up for the delta.
>
> -Vineet
>

Re: RISC-V: Folding memory for FP + constant case

[Vineet Gupta]

On 8/1/23 16:06, Philipp Tomsich wrote:
> Very helpful! Looks as if regprop for stack_pointer is now either too
> conservative — or one of our patches is missing in everyone's test
> setup; we'll take a closer look.

FWIW, all 5 of them involve a SH2ADD have SP as source in the fold FP 
case which f-m-o seems to be generating a MV for.

>
> On Wed, 2 Aug 2023 at 01:03, Vineet Gupta <vineetg@rivosinc.com> wrote:
>>
>>
>> On 8/1/23 15:07, Philipp Tomsich wrote:
>>> +Manolis Tsamis
>>>
>>> On Tue, 1 Aug 2023 at 23:56, Jeff Law via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>
>>>> On 8/1/23 13:14, Vineet Gupta wrote:
>>>>
>>>>> I have some numbers for f-m-o v3 vs this. Attached here (vs. inline to
>>>>> avoid the Thunderbird mangling the test formatting)
>>>> Thanks.  Of particular importance is the leela change.  My recollection
>>>> was that the f-m-o work also picked up that case.  But if my memory is
>>>> faulty (always a possibility), then that shows a clear case where
>>>> Jivan's work picks up a case not handled by Manolis's work.
>>> f-m-o originally targeted (and benefited) the leela-case.  I wonder if
>>> other optimizations/changes over the last year interfere with this and
>>> what needs to be changed to accomodate this... looks like we need to
>>> revisit against trunk.
>>>
>>> Philipp.
>>>
>>>> And on the other direction we can see that deepsjeng isn't helped by
>>>> Jivan's work, but is helped by Manolis's new pass.
>>>>
>>>> I'd always hoped/expected we'd have cases where one patch clearly helped
>>>> over the other.  While the .25% to .37% improvements for the three most
>>>> impacted benchmarks doesn't move the needle much across the whole suite
>>>> they do add up over time.
>>>>
>>>> Jeff
>> I took a quick look at Leela, the significant difference is from
>> additional insns with SP not getting propagated.
>>
>> e.g.
>>
>>      231b6:    mv    a4,sp
>>      231b8:    sh2add    a5,a5,a4
>>
>> vs.
>>
>>      1e824:    sh2add    a5,a5,sp
>>
>> There are 5 such instances which more or less make up for the delta.
>>
>> -Vineet
>>

Re: RISC-V: Folding memory for FP + constant case

[Jeff Law]

On 8/1/23 17:13, Vineet Gupta wrote:
> 
> On 8/1/23 16:06, Philipp Tomsich wrote:
>> Very helpful! Looks as if regprop for stack_pointer is now either too
>> conservative — or one of our patches is missing in everyone's test
>> setup; we'll take a closer look.
> 
> FWIW, all 5 of them involve a SH2ADD have SP as source in the fold FP 
> case which f-m-o seems to be generating a MV for.
To clarify f-m-o isn't generating the mv.  It's simplifying a sequence 
by pushing the constant in an addi instruction into the memory 
reference.  As a result the addi simplifies into a sp->reg copy that is 
supposed to then be propagated away.

Also note that getting FP out of the shift-add sequences is the other 
key goal of Jivan's work.  FP elimination always results in a 
spill/reload if we have a shift-add insn where one operand is FP.



jeff

Re: RISC-V: Folding memory for FP + constant case

[Vineet Gupta]

On 8/1/23 16:27, Jeff Law wrote:
>
>
> On 8/1/23 17:13, Vineet Gupta wrote:
>>
>> On 8/1/23 16:06, Philipp Tomsich wrote:
>>> Very helpful! Looks as if regprop for stack_pointer is now either too
>>> conservative — or one of our patches is missing in everyone's test
>>> setup; we'll take a closer look.
>>
>> FWIW, all 5 of them involve a SH2ADD have SP as source in the fold FP 
>> case which f-m-o seems to be generating a MV for.
> To clarify f-m-o isn't generating the mv.  It's simplifying a sequence 
> by pushing the constant in an addi instruction into the memory 
> reference.  As a result the addi simplifies into a sp->reg copy that 
> is supposed to then be propagated away.

Yep, that's clear.

>
> Also note that getting FP out of the shift-add sequences is the other 
> key goal of Jivan's work.  FP elimination always results in a 
> spill/reload if we have a shift-add insn where one operand is FP. 

Hmm, are you saying it should NOT be generating shift-add with SP as 
src, because currently thats exactly what fold FP offset *is* doing and 
is the reason it has 5 less insns.

-Vineet

Re: RISC-V: Folding memory for FP + constant case

[Jeff Law]

On 8/1/23 17:38, Vineet Gupta wrote:
>>
>> Also note that getting FP out of the shift-add sequences is the other 
>> key goal of Jivan's work.  FP elimination always results in a 
>> spill/reload if we have a shift-add insn where one operand is FP. 
> 
> Hmm, are you saying it should NOT be generating shift-add with SP as 
> src, because currently thats exactly what fold FP offset *is* doing and 
> is the reason it has 5 less insns.
We should not have shift-add with FP as a source prior to register 
allocation because it will almost always generate spill code.


jeff

Re: RISC-V: Folding memory for FP + constant case

[Manolis Tsamis]

Hi all,

It is true that regcprop currently does not propagate sp and hence
leela is not optimized, but from what I see this should be something
we can address.

The reason that the propagation fails is this check that I have added
when I introduced maybe_copy_reg_attrs:

else if (REG_POINTER (new_reg) != REG_POINTER (old_reg))
  {
    /* Only a single instance of STACK_POINTER_RTX must exist and we cannot
       modify it. Allow propagation if REG_POINTER for OLD_REG matches and
       don't touch ORIGINAL_REGNO and REG_ATTRS. */
    return NULL_RTX;
  }

To be honest I did add this back then just to be on the safe side of
whether a mismatch in REG_POINTER after propagation would be an issue
(since the original regcprop had caused enough problems).

I see two ways to solve this and make fmo able to optimize leela as well:
 1) Remove the REG_POINTER check in regcprop if that is safe. My
understanding is that REG_POINTER is used as a hint and there would be
no correctness issues.
 2) Mark the corresponding registers with REG_POINTER. I'm not sure
where that is supposed to happen.

Since the instructions look like this:
  (insn 113 11 16 2 (set (reg:DI 15 a5 [226])
          (reg/f:DI 2 sp)) 179 {*movdi_64bit}
       (nil))

I assume that we'd want to mark a5 as REG_POINTER anyway (which is
not), and in that case propagation would work.
On the other hand if there's no correctness issue w.r.t. REG_POINTER
and regcprop then removing the additional check would increase
propagation opportunities in general which is also good.

Thanks,
Manolis

On Wed, Aug 2, 2023 at 2:52 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 8/1/23 17:38, Vineet Gupta wrote:
> >>
> >> Also note that getting FP out of the shift-add sequences is the other
> >> key goal of Jivan's work.  FP elimination always results in a
> >> spill/reload if we have a shift-add insn where one operand is FP.
> >
> > Hmm, are you saying it should NOT be generating shift-add with SP as
> > src, because currently thats exactly what fold FP offset *is* doing and
> > is the reason it has 5 less insns.
> We should not have shift-add with FP as a source prior to register
> allocation because it will almost always generate spill code.
>
>
> jeff

Re: RISC-V: Folding memory for FP + constant case

[Jeff Law]

On 8/4/23 03:52, Manolis Tsamis wrote:
> Hi all,
> 
> It is true that regcprop currently does not propagate sp and hence
> leela is not optimized, but from what I see this should be something
> we can address.
> 
> The reason that the propagation fails is this check that I have added
> when I introduced maybe_copy_reg_attrs:
> 
> else if (REG_POINTER (new_reg) != REG_POINTER (old_reg))
>    {
>      /* Only a single instance of STACK_POINTER_RTX must exist and we cannot
>         modify it. Allow propagation if REG_POINTER for OLD_REG matches and
>         don't touch ORIGINAL_REGNO and REG_ATTRS. */
>      return NULL_RTX;
>    }
> 
> To be honest I did add this back then just to be on the safe side of
> whether a mismatch in REG_POINTER after propagation would be an issue
> (since the original regcprop had caused enough problems).
No worries.  Obviously not propagating is the safe thing to do and if we 
find notable missed cases we can always refine the existing code like 
we're considering now.

> 
> I see two ways to solve this and make fmo able to optimize leela as well:
>   1) Remove the REG_POINTER check in regcprop if that is safe. My
> understanding is that REG_POINTER is used as a hint and there would be
> no correctness issues.
REG_POINTER is meant to be conservatively correct.  If REG_POINTER is 
set, then the register must be a pointer to a valid memory location.  If 
REG_POINTER is not set, then it may or may not point to a valid memory 
location.  sp, fp, ap are by definition pointers and thus have 
REG_POINTER set.

With that definition we can safely copy propagate away an sp->gpr copy 
from a REG_POINTER standpoint.










>   2) Mark the corresponding registers with REG_POINTER. I'm not sure
> where that is supposed to happen.
> 
> Since the instructions look like this:
>    (insn 113 11 16 2 (set (reg:DI 15 a5 [226])
>            (reg/f:DI 2 sp)) 179 {*movdi_64bit}
>         (nil))
> 
> I assume that we'd want to mark a5 as REG_POINTER anyway (which is
> not), and in that case propagation would work.
I'd strongly recommend against that.  The problem is the destination 
register might have been used in another context as an index register. 
We've fixed a few bugs in this space through the years I suspect there 
may be others lurking.  You can't directly set that up in C code, but 
once you get down to RTL it can happen.


> On the other hand if there's no correctness issue w.r.t. REG_POINTER
> and regcprop then removing the additional check would increase
> propagation opportunities in general which is also good.
I think you can safely remove this limitation.

jeff

Re: RISC-V: Folding memory for FP + constant case

[Manolis Tsamis]

Hi Jeff,

Thanks for all the info!
Then I'll prepare/test a patch that removes this regcprop limitation
and send it out.
I have already tested that this change is enough to make fmo optimize
leela as well.

Manolis

On Fri, Aug 4, 2023 at 7:23 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 8/4/23 03:52, Manolis Tsamis wrote:
> > Hi all,
> >
> > It is true that regcprop currently does not propagate sp and hence
> > leela is not optimized, but from what I see this should be something
> > we can address.
> >
> > The reason that the propagation fails is this check that I have added
> > when I introduced maybe_copy_reg_attrs:
> >
> > else if (REG_POINTER (new_reg) != REG_POINTER (old_reg))
> >    {
> >      /* Only a single instance of STACK_POINTER_RTX must exist and we cannot
> >         modify it. Allow propagation if REG_POINTER for OLD_REG matches and
> >         don't touch ORIGINAL_REGNO and REG_ATTRS. */
> >      return NULL_RTX;
> >    }
> >
> > To be honest I did add this back then just to be on the safe side of
> > whether a mismatch in REG_POINTER after propagation would be an issue
> > (since the original regcprop had caused enough problems).
> No worries.  Obviously not propagating is the safe thing to do and if we
> find notable missed cases we can always refine the existing code like
> we're considering now.
>
> >
> > I see two ways to solve this and make fmo able to optimize leela as well:
> >   1) Remove the REG_POINTER check in regcprop if that is safe. My
> > understanding is that REG_POINTER is used as a hint and there would be
> > no correctness issues.
> REG_POINTER is meant to be conservatively correct.  If REG_POINTER is
> set, then the register must be a pointer to a valid memory location.  If
> REG_POINTER is not set, then it may or may not point to a valid memory
> location.  sp, fp, ap are by definition pointers and thus have
> REG_POINTER set.
>
> With that definition we can safely copy propagate away an sp->gpr copy
> from a REG_POINTER standpoint.
>
>
>
>
>
>
>
>
>
>
> >   2) Mark the corresponding registers with REG_POINTER. I'm not sure
> > where that is supposed to happen.
> >
> > Since the instructions look like this:
> >    (insn 113 11 16 2 (set (reg:DI 15 a5 [226])
> >            (reg/f:DI 2 sp)) 179 {*movdi_64bit}
> >         (nil))
> >
> > I assume that we'd want to mark a5 as REG_POINTER anyway (which is
> > not), and in that case propagation would work.
> I'd strongly recommend against that.  The problem is the destination
> register might have been used in another context as an index register.
> We've fixed a few bugs in this space through the years I suspect there
> may be others lurking.  You can't directly set that up in C code, but
> once you get down to RTL it can happen.
>
>
> > On the other hand if there's no correctness issue w.r.t. REG_POINTER
> > and regcprop then removing the additional check would increase
> > propagation opportunities in general which is also good.
> I think you can safely remove this limitation.
>
> jeff

Re: RISC-V: Folding memory for FP + constant case

[Jeff Law]

On 8/1/23 17:06, Philipp Tomsich wrote:
> Very helpful! Looks as if regprop for stack_pointer is now either too
> conservative — or one of our patches is missing in everyone's test
> setup; we'll take a closer look.
They must not be working as expected or folks are using old trees. 
Manolis's work for regcprop has been on the trunk for about 5-6 weeks ag 
this point:


> commit 893883f2f8f56984209c6ed210ee992ff71a14b0
> Author: Manolis Tsamis <manolis.tsamis@vrull.eu>
> Date:   Tue Jun 20 16:23:52 2023 +0200
> 
>     cprop_hardreg: fix ORIGINAL_REGNO/REG_ATTRS/REG_POINTER handling
>     
>     Fixes: 6a2e8dcbbd4bab3
>     
>     Propagation for the stack pointer in regcprop was enabled in
>     6a2e8dcbbd4bab3, but set ORIGINAL_REGNO/REG_ATTRS/REG_POINTER for
>     stack_pointer_rtx which caused regression (e.g., PR 110313, PR 110308).
>     
>     This fix adds special handling for stack_pointer_rtx in the places
>     where maybe_mode_change is called. This also adds an check in
>     maybe_mode_change to return the stack pointer only when the requested
>     mode matches the mode of stack_pointer_rtx.
>     
>             PR debug/110308
>     
>     gcc/ChangeLog:
>     
>             * regcprop.cc (maybe_mode_change): Check stack_pointer_rtx mode.
>             (maybe_copy_reg_attrs): New function.
>             (find_oldest_value_reg): Use maybe_copy_reg_attrs.
>             (copyprop_hardreg_forward_1): Ditto.
>     
>     gcc/testsuite/ChangeLog:
>     
>             * g++.dg/torture/pr110308.C: New test.
>     
>     Signed-off-by: Manolis Tsamis <manolis.tsamis@vrull.eu>
>     Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> 
> commit 6a2e8dcbbd4bab374b27abea375bf7a921047800
> Author: Manolis Tsamis <manolis.tsamis@vrull.eu>
> Date:   Thu May 25 13:44:41 2023 +0200
> 
>     cprop_hardreg: Enable propagation of the stack pointer if possible
>     
>     Propagation of the stack pointer in cprop_hardreg is currenty
>     forbidden in all cases, due to maybe_mode_change returning NULL.
>     Relax this restriction and allow propagation when no mode change is
>     requested.
>     
>     gcc/ChangeLog:
>     
>             * regcprop.cc (maybe_mode_change): Enable stack pointer
>             propagation.

Re: RISC-V: Folding memory for FP + constant case

[Vineet Gupta]

On 8/1/23 16:22, Jeff Law wrote:
> They must not be working as expected or folks are using old trees. 
> Manolis's work for regcprop has been on the trunk for about 5-6 weeks 
> ag this point: 

I have bleeding edge trunk from 2-3 days back. I think we are looking 
for the following which the tree does have.

2023-05-25 6a2e8dcbbd4b cprop_hardreg: Enable propagation of the stack 
pointer if possible

-Vineet

Re: RISC-V: Folding memory for FP + constant case

[Jeff Law]

On 8/1/23 17:03, Vineet Gupta wrote:
> 
> 
> On 8/1/23 15:07, Philipp Tomsich wrote:
>> +Manolis Tsamis
>>
>> On Tue, 1 Aug 2023 at 23:56, Jeff Law via Gcc-patches
>> <gcc-patches@gcc.gnu.org> wrote:
>>>
>>>
>>> On 8/1/23 13:14, Vineet Gupta wrote:
>>>
>>>> I have some numbers for f-m-o v3 vs this. Attached here (vs. inline to
>>>> avoid the Thunderbird mangling the test formatting)
>>> Thanks.  Of particular importance is the leela change.  My recollection
>>> was that the f-m-o work also picked up that case.  But if my memory is
>>> faulty (always a possibility), then that shows a clear case where
>>> Jivan's work picks up a case not handled by Manolis's work.
>> f-m-o originally targeted (and benefited) the leela-case.  I wonder if
>> other optimizations/changes over the last year interfere with this and
>> what needs to be changed to accomodate this... looks like we need to
>> revisit against trunk.
>>
>> Philipp.
>>
>>> And on the other direction we can see that deepsjeng isn't helped by
>>> Jivan's work, but is helped by Manolis's new pass.
>>>
>>> I'd always hoped/expected we'd have cases where one patch clearly helped
>>> over the other.  While the .25% to .37% improvements for the three most
>>> impacted benchmarks doesn't move the needle much across the whole suite
>>> they do add up over time.
>>>
>>> Jeff
> 
> I took a quick look at Leela, the significant difference is from 
> additional insns with SP not getting propagated.
> 
> e.g.
> 
>     231b6:    mv    a4,sp
>     231b8:    sh2add    a5,a5,a4
> 
> vs.
> 
>     1e824:    sh2add    a5,a5,sp
> 
> There are 5 such instances which more or less make up for the delta.
ACK.  Jivan and I have seen similar things with some other work in this 
space.  What's weird is the bits of Manolis's work that went in a month 
or so ago are supposed to address this exact issue in the post-reload 
const/copy propagation pass.

Jeff

Re: RISC-V: Folding memory for FP + constant case

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 2520 bytes --]



On 7/12/23 14:59, Jivan Hakobyan via Gcc-patches wrote:
> Accessing local arrays element turned into load form (fp + (index << C1)) +
> C2 address.
> In the case when access is in the loop we got loop invariant computation.
> For some reason, moving out that part cannot be done in
> loop-invariant passes.
> But we can handle that in target-specific hook (legitimize_address).
> That provides an opportunity to rewrite memory access more suitable for the
> target architecture.
> 
> This patch solves the mentioned case by rewriting mentioned case to ((fp +
> C2) + (index << C1))
> I have evaluated it on SPEC2017 and got an improvement on leela (over 7b
> instructions,
> .39% of the dynamic count) and dwarfs the regression for gcc (14m
> instructions, .0012%
> of the dynamic count).
> 
> 
> gcc/ChangeLog:
>          * config/riscv/riscv.cc (riscv_legitimize_address): Handle folding.
>          (mem_shadd_or_shadd_rtx_p): New predicate.
So I poked a bit more in this space today.

As you may have noted, Manolis's patch still needs another rev.  But I 
was able to test this patch in conjunction with the f-m-o patch as well 
as the additional improvements made to hard register cprop.  The net 
result was that this patch still shows a nice decrease in instruction 
counts on leela.  It's a bit of a mixed bag elsewhere.

I dove a bit deeper into the small regression in x264.  In the case I 
looked at the reason the patch regresses is the original form of the 
address calculations exposes a common subexpression ie

addr1 = (reg1 << 2) + fp + C1
addr2 = (reg1 << 2) + fp + C2

(reg1 << 2) + fp is a common subexpression resulting in something like 
this as we leave CSE:

t = (reg1 << 2) + fp;
addr1 = t + C1
addr2 = t + C2
mem (addr1)
mem (addr2)

C1 and C2 are small constants, so combine generates

t = (reg1 << 2) + fp;
mem (t+C1)
mem (t+C2)

FP elimination occurs after IRA and we get:

t2 = sp + C3
t = (reg << 2) + t2
mem (t + C1)
mem (t + C2)


Not bad.  Manolis's work should allow us to improve that a bit more.


With this patch we don't capture the CSE and ultimately generate 
slightly worse code.  This kind of issue is fairly inherent in 
reassociations -- and given the regression is 2 orders of magnitude 
smaller than the improvement my inclination is to go forward with this 
patch.



I've fixed a few formatting issues and changed once conditional to use 
CONST_INT_P rather than checking the code directory and pushed the final 
version to the trunk.

Thanks for your patience.

jeff

[-- Attachment #2: P --]
[-- Type: text/plain, Size: 3381 bytes --]

commit a16dc729fda9fabd6472d50cce45791cb3b6ada8
Author: Jivan Hakobyan <jivanhakobyan9@gmail.com>
Date:   Wed Aug 9 13:26:58 2023 -0600

    RISC-V: Folding memory for FP + constant case
    
    Accessing local arrays element turned into load form (fp + (index << C1)) +
    C2 address.
    
    In the case when access is in the loop we got loop invariant computation.  For
    some reason, moving out that part cannot be done in loop-invariant passes.  But
    we can handle that in target-specific hook (legitimize_address).  That provides
    an opportunity to rewrite memory access more suitable for the target
    architecture.
    
    This patch solves the mentioned case by rewriting mentioned case to ((fp +
    C2) + (index << C1))
    
    I have evaluated it on SPEC2017 and got an improvement on leela (over 7b
    instructions, .39% of the dynamic count) and dwarfs the regression for gcc (14m
    instructions, .0012% of the dynamic count).
    
    gcc/ChangeLog:
            * config/riscv/riscv.cc (riscv_legitimize_address): Handle folding.
            (mem_shadd_or_shadd_rtx_p): New function.

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 77892da2920..7f2041a54ba 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -1805,6 +1805,22 @@ riscv_shorten_lw_offset (rtx base, HOST_WIDE_INT offset)
   return addr;
 }
 
+/* Helper for riscv_legitimize_address. Given X, return true if it
+   is a left shift by 1, 2 or 3 positions or a multiply by 2, 4 or 8.
+
+   This respectively represent canonical shift-add rtxs or scaled
+   memory addresses.  */
+static bool
+mem_shadd_or_shadd_rtx_p (rtx x)
+{
+  return ((GET_CODE (x) == ASHIFT
+	   || GET_CODE (x) == MULT)
+	  && CONST_INT_P (XEXP (x, 1))
+	  && ((GET_CODE (x) == ASHIFT && IN_RANGE (INTVAL (XEXP (x, 1)), 1, 3))
+	      || (GET_CODE (x) == MULT
+		  && IN_RANGE (exact_log2 (INTVAL (XEXP (x, 1))), 1, 3))));
+}
+
 /* This function is used to implement LEGITIMIZE_ADDRESS.  If X can
    be legitimized in a way that the generic machinery might not expect,
    return a new address, otherwise return NULL.  MODE is the mode of
@@ -1830,6 +1846,32 @@ riscv_legitimize_address (rtx x, rtx oldx ATTRIBUTE_UNUSED,
       rtx base = XEXP (x, 0);
       HOST_WIDE_INT offset = INTVAL (XEXP (x, 1));
 
+      /* Handle (plus (plus (mult (a) (mem_shadd_constant)) (fp)) (C)) case.  */
+      if (GET_CODE (base) == PLUS && mem_shadd_or_shadd_rtx_p (XEXP (base, 0))
+	  && SMALL_OPERAND (offset))
+	{
+	  rtx index = XEXP (base, 0);
+	  rtx fp = XEXP (base, 1);
+	  if (REGNO (fp) == VIRTUAL_STACK_VARS_REGNUM)
+	    {
+
+	      /* If we were given a MULT, we must fix the constant
+		 as we're going to create the ASHIFT form.  */
+	      int shift_val = INTVAL (XEXP (index, 1));
+	      if (GET_CODE (index) == MULT)
+		shift_val = exact_log2 (shift_val);
+
+	      rtx reg1 = gen_reg_rtx (Pmode);
+	      rtx reg2 = gen_reg_rtx (Pmode);
+	      rtx reg3 = gen_reg_rtx (Pmode);
+	      riscv_emit_binary (PLUS, reg1, fp, GEN_INT (offset));
+	      riscv_emit_binary (ASHIFT, reg2, XEXP (index, 0), GEN_INT (shift_val));
+	      riscv_emit_binary (PLUS, reg3, reg2, reg1);
+
+	      return reg3;
+	    }
+	}
+
       if (!riscv_valid_base_register_p (base, mode, false))
 	base = copy_to_mode_reg (Pmode, base);
       if (optimize_function_for_size_p (cfun)