RISCV: Implement isLoadFromStackSlot/isStoreToStackSlot for rvv #120524

arsenm · 2024-12-19T05:32:37Z

This partially helps avoid regressions in a future regalloc patch.
It isn't sufficient, and I think there are more missing implementations
of the copy and spill hooks.

arsenm · 2024-12-19T05:32:56Z

RISCV: Implement isLoadFromStackSlot/isStoreToStackSlot for rvv #120524 👈 (View in Graphite)
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

llvmbot · 2024-12-19T05:33:10Z

@llvm/pr-subscribers-backend-risc-v

Author: Matt Arsenault (arsenm)

Changes

This partially helps avoid regressions in a future regalloc patch.
It isn't sufficient, and I think there are more missing implementations
of the copy and spill hooks.

Full diff: https://github.com/llvm/llvm-project/pull/120524.diff

6 Files Affected:

(modified) llvm/lib/Target/RISCV/RISCVInstrInfo.cpp (+12)
(modified) llvm/test/CodeGen/RISCV/rvv/cttz-vp.ll (+3-3)
(modified) llvm/test/CodeGen/RISCV/rvv/fixed-vectors-ctlz-vp.ll (+8-8)
(modified) llvm/test/CodeGen/RISCV/rvv/fixed-vectors-interleaved-access.ll (+5-5)
(modified) llvm/test/CodeGen/RISCV/rvv/fshr-fshl-vp.ll (+2-2)
(modified) llvm/test/CodeGen/RISCV/rvv/vfma-vp.ll (+5-5)

diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
index 7e0063589b6f4c..3321449f8781cd 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
@@ -118,6 +118,12 @@ Register RISCVInstrInfo::isLoadFromStackSlot(const MachineInstr &MI,
   case RISCV::FLD:
     MemBytes = 8;
     break;
+  case RISCV::VL8RE8_V:
+    if (!MI.getOperand(1).isFI())
+      return Register();
+    FrameIndex = MI.getOperand(1).getIndex();
+    MemBytes = ~0u;
+    return MI.getOperand(0).getReg();
   }
 
   if (MI.getOperand(1).isFI() && MI.getOperand(2).isImm() &&
@@ -158,6 +164,12 @@ Register RISCVInstrInfo::isStoreToStackSlot(const MachineInstr &MI,
   case RISCV::FSD:
     MemBytes = 8;
     break;
+  case RISCV::VS8R_V:
+    if (!MI.getOperand(1).isFI())
+      return Register();
+    FrameIndex = MI.getOperand(1).getIndex();
+    MemBytes = ~0u;
+    return MI.getOperand(0).getReg();
   }
 
   if (MI.getOperand(1).isFI() && MI.getOperand(2).isImm() &&
diff --git a/llvm/test/CodeGen/RISCV/rvv/cttz-vp.ll b/llvm/test/CodeGen/RISCV/rvv/cttz-vp.ll
index 766717d92a7493..2386f0b8b9b253 100644
--- a/llvm/test/CodeGen/RISCV/rvv/cttz-vp.ll
+++ b/llvm/test/CodeGen/RISCV/rvv/cttz-vp.ll
@@ -2339,14 +2339,14 @@ define <vscale x 16 x i64> @vp_cttz_nxv16i64(<vscale x 16 x i64> %va, <vscale x
 ; RV32-NEXT:    add a4, sp, a4
 ; RV32-NEXT:    addi a4, a4, 16
 ; RV32-NEXT:    vs8r.v v8, (a4) # Unknown-size Folded Spill
-; RV32-NEXT:    vsrl.vi v16, v16, 2, v0.t
+; RV32-NEXT:    vsrl.vi v8, v16, 2, v0.t
 ; RV32-NEXT:    csrr a4, vlenb
 ; RV32-NEXT:    li a5, 48
 ; RV32-NEXT:    mul a4, a4, a5
 ; RV32-NEXT:    add a4, sp, a4
 ; RV32-NEXT:    addi a4, a4, 16
-; RV32-NEXT:    vl8r.v v8, (a4) # Unknown-size Folded Reload
-; RV32-NEXT:    vand.vv v16, v16, v8, v0.t
+; RV32-NEXT:    vl8r.v v16, (a4) # Unknown-size Folded Reload
+; RV32-NEXT:    vand.vv v16, v8, v16, v0.t
 ; RV32-NEXT:    csrr a4, vlenb
 ; RV32-NEXT:    li a5, 24
 ; RV32-NEXT:    mul a4, a4, a5
diff --git a/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-ctlz-vp.ll b/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-ctlz-vp.ll
index 9d0d42cf754c5e..7f4b3570263a7d 100644
--- a/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-ctlz-vp.ll
+++ b/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-ctlz-vp.ll
@@ -2292,17 +2292,17 @@ define <32 x i64> @vp_ctlz_v32i64(<32 x i64> %va, <32 x i1> %m, i32 zeroext %evl
 ; RV32-NEXT:    addi a0, a0, 48
 ; RV32-NEXT:    vs8r.v v8, (a0) # Unknown-size Folded Spill
 ; RV32-NEXT:    csrr a0, vlenb
-; RV32-NEXT:    li a1, 48
+; RV32-NEXT:    li a1, 40
 ; RV32-NEXT:    mul a0, a0, a1
 ; RV32-NEXT:    add a0, sp, a0
 ; RV32-NEXT:    addi a0, a0, 48
-; RV32-NEXT:    vl8r.v v8, (a0) # Unknown-size Folded Reload
+; RV32-NEXT:    vl8r.v v16, (a0) # Unknown-size Folded Reload
 ; RV32-NEXT:    csrr a0, vlenb
-; RV32-NEXT:    li a1, 40
+; RV32-NEXT:    li a1, 48
 ; RV32-NEXT:    mul a0, a0, a1
 ; RV32-NEXT:    add a0, sp, a0
 ; RV32-NEXT:    addi a0, a0, 48
-; RV32-NEXT:    vl8r.v v16, (a0) # Unknown-size Folded Reload
+; RV32-NEXT:    vl8r.v v8, (a0) # Unknown-size Folded Reload
 ; RV32-NEXT:    vand.vv v16, v16, v8, v0.t
 ; RV32-NEXT:    csrr a0, vlenb
 ; RV32-NEXT:    slli a0, a0, 4
@@ -4998,17 +4998,17 @@ define <32 x i64> @vp_ctlz_zero_undef_v32i64(<32 x i64> %va, <32 x i1> %m, i32 z
 ; RV32-NEXT:    addi a0, a0, 48
 ; RV32-NEXT:    vs8r.v v8, (a0) # Unknown-size Folded Spill
 ; RV32-NEXT:    csrr a0, vlenb
-; RV32-NEXT:    li a1, 48
+; RV32-NEXT:    li a1, 40
 ; RV32-NEXT:    mul a0, a0, a1
 ; RV32-NEXT:    add a0, sp, a0
 ; RV32-NEXT:    addi a0, a0, 48
-; RV32-NEXT:    vl8r.v v8, (a0) # Unknown-size Folded Reload
+; RV32-NEXT:    vl8r.v v16, (a0) # Unknown-size Folded Reload
 ; RV32-NEXT:    csrr a0, vlenb
-; RV32-NEXT:    li a1, 40
+; RV32-NEXT:    li a1, 48
 ; RV32-NEXT:    mul a0, a0, a1
 ; RV32-NEXT:    add a0, sp, a0
 ; RV32-NEXT:    addi a0, a0, 48
-; RV32-NEXT:    vl8r.v v16, (a0) # Unknown-size Folded Reload
+; RV32-NEXT:    vl8r.v v8, (a0) # Unknown-size Folded Reload
 ; RV32-NEXT:    vand.vv v16, v16, v8, v0.t
 ; RV32-NEXT:    csrr a0, vlenb
 ; RV32-NEXT:    slli a0, a0, 4
diff --git a/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-interleaved-access.ll b/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-interleaved-access.ll
index 67d649902b022a..5592594f897030 100644
--- a/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-interleaved-access.ll
+++ b/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-interleaved-access.ll
@@ -343,16 +343,16 @@ define {<8 x i64>, <8 x i64>, <8 x i64>, <8 x i64>, <8 x i64>, <8 x i64>} @load_
 ; RV32-NEXT:    vs4r.v v8, (a1) # Unknown-size Folded Spill
 ; RV32-NEXT:    vmv1r.v v0, v14
 ; RV32-NEXT:    csrr a1, vlenb
-; RV32-NEXT:    li a3, 72
-; RV32-NEXT:    mul a1, a1, a3
+; RV32-NEXT:    slli a1, a1, 6
 ; RV32-NEXT:    add a1, sp, a1
 ; RV32-NEXT:    addi a1, a1, 16
-; RV32-NEXT:    vl8r.v v16, (a1) # Unknown-size Folded Reload
+; RV32-NEXT:    vl8r.v v8, (a1) # Unknown-size Folded Reload
 ; RV32-NEXT:    csrr a1, vlenb
-; RV32-NEXT:    slli a1, a1, 6
+; RV32-NEXT:    li a3, 72
+; RV32-NEXT:    mul a1, a1, a3
 ; RV32-NEXT:    add a1, sp, a1
 ; RV32-NEXT:    addi a1, a1, 16
-; RV32-NEXT:    vl8r.v v8, (a1) # Unknown-size Folded Reload
+; RV32-NEXT:    vl8r.v v16, (a1) # Unknown-size Folded Reload
 ; RV32-NEXT:    vsetvli zero, a2, e32, m8, ta, ma
 ; RV32-NEXT:    vmerge.vvm v16, v8, v16, v0
 ; RV32-NEXT:    csrr a1, vlenb
diff --git a/llvm/test/CodeGen/RISCV/rvv/fshr-fshl-vp.ll b/llvm/test/CodeGen/RISCV/rvv/fshr-fshl-vp.ll
index b569efc7447da6..70445929259400 100644
--- a/llvm/test/CodeGen/RISCV/rvv/fshr-fshl-vp.ll
+++ b/llvm/test/CodeGen/RISCV/rvv/fshr-fshl-vp.ll
@@ -1109,9 +1109,9 @@ define <vscale x 16 x i64> @fshr_v16i64(<vscale x 16 x i64> %a, <vscale x 16 x i
 ; CHECK-NEXT:    mul a0, a0, a1
 ; CHECK-NEXT:    add a0, sp, a0
 ; CHECK-NEXT:    addi a0, a0, 16
-; CHECK-NEXT:    vl8r.v v16, (a0) # Unknown-size Folded Reload
+; CHECK-NEXT:    vl8r.v v8, (a0) # Unknown-size Folded Reload
 ; CHECK-NEXT:    vsetvli zero, a4, e64, m8, ta, ma
-; CHECK-NEXT:    vand.vx v8, v16, a3, v0.t
+; CHECK-NEXT:    vand.vx v8, v8, a3, v0.t
 ; CHECK-NEXT:    csrr a0, vlenb
 ; CHECK-NEXT:    slli a0, a0, 4
 ; CHECK-NEXT:    add a0, sp, a0
diff --git a/llvm/test/CodeGen/RISCV/rvv/vfma-vp.ll b/llvm/test/CodeGen/RISCV/rvv/vfma-vp.ll
index ab67e9833c78aa..35e8a850510c3e 100644
--- a/llvm/test/CodeGen/RISCV/rvv/vfma-vp.ll
+++ b/llvm/test/CodeGen/RISCV/rvv/vfma-vp.ll
@@ -10357,17 +10357,17 @@ define <vscale x 32 x half> @vfnmadd_vf_nxv32f16_commute(<vscale x 32 x half> %v
 ; ZVFHMIN-NEXT:    vfwcvt.f.f.v v8, v16
 ; ZVFHMIN-NEXT:    csrr a4, vlenb
 ; ZVFHMIN-NEXT:    slli a4, a4, 3
-; ZVFHMIN-NEXT:    mv a5, a4
-; ZVFHMIN-NEXT:    slli a4, a4, 1
-; ZVFHMIN-NEXT:    add a4, a4, a5
 ; ZVFHMIN-NEXT:    add a4, sp, a4
 ; ZVFHMIN-NEXT:    addi a4, a4, 16
-; ZVFHMIN-NEXT:    vl8r.v v16, (a4) # Unknown-size Folded Reload
+; ZVFHMIN-NEXT:    vl8r.v v24, (a4) # Unknown-size Folded Reload
 ; ZVFHMIN-NEXT:    csrr a4, vlenb
 ; ZVFHMIN-NEXT:    slli a4, a4, 3
+; ZVFHMIN-NEXT:    mv a5, a4
+; ZVFHMIN-NEXT:    slli a4, a4, 1
+; ZVFHMIN-NEXT:    add a4, a4, a5
 ; ZVFHMIN-NEXT:    add a4, sp, a4
 ; ZVFHMIN-NEXT:    addi a4, a4, 16
-; ZVFHMIN-NEXT:    vl8r.v v24, (a4) # Unknown-size Folded Reload
+; ZVFHMIN-NEXT:    vl8r.v v16, (a4) # Unknown-size Folded Reload
 ; ZVFHMIN-NEXT:    vsetvli zero, a3, e32, m8, ta, ma
 ; ZVFHMIN-NEXT:    vfmadd.vv v16, v8, v24, v0.t
 ; ZVFHMIN-NEXT:    csrr a3, vlenb

wangpc-pp · 2024-12-19T05:41:43Z

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

@@ -118,6 +118,12 @@ Register RISCVInstrInfo::isLoadFromStackSlot(const MachineInstr &MI,
  case RISCV::FLD:
    MemBytes = 8;
    break;
+  case RISCV::VL8RE8_V:


What about 1/2/4? And also Spill/Reload pseudos?

Not all the sizes showed up in tests with asserts. I didn't see any of the spill/reload pseudos appear either

Oh, you just added them! We still need handle PseudoVSPILL/PseudoVRELOAD I think.

I never see the spill pseudos appear here for some reason

We may don't have ehough test coverages for segment types.

FYI, this includes all of the instructions we actually use for reloading vector registers from the stack. There are other instructions we could use, but not that we do use. (Excluding the segment spill stuff as it's own sub-problem.)

wangpc-pp · 2024-12-19T07:00:09Z

This partially helps avoid regressions in a future regalloc patch.

Which regalloc patch?

arsenm · 2024-12-19T07:00:31Z

Which regalloc patch?

One I haven't posted or even decided if it's the correct fix

arsenm · 2025-01-07T06:40:05Z

ping, though the patch I wanted this for is probably a dead end

topperc · 2025-01-07T19:55:17Z

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

+    if (!MI.getOperand(1).isFI())
+      return Register();
+    FrameIndex = MI.getOperand(1).getIndex();
+    MemBytes = ~0u;


Looks like StackSlotColoring is one of the only callers that use interface with MemBytes, but it doesn't look like it treats ~Ou as unknown.

The only usage is to compare LoadSize and StoreSize, so it may not matter?

if (FirstSS != SecondSS || LoadReg != StoreReg || FirstSS == -1 || LoadSize != StoreSize || !MFI->isSpillSlotObjectIndex(FirstSS)) continue;

Or we can use the getRealMinVLen() to calculate the memory size.

Could also change the TTI signature to use TypeSize

Split off in: #132244

This partially helps avoid regressions in a future regalloc patch. It isn't sufficient, and I think there are more missing implementations of the copy and spill hooks.

…ad/mayStore. One test looks like a regression, which is probably rematerialization not being handled for something.

arsenm

@topperc @wangpc-pp could someone who knows about riscv pick this one up for me?

arsenm · 2025-03-14T04:18:41Z

@preames want to adopt this if you're touching RISCV regalloc?

preames · 2025-03-17T19:50:14Z

@preames want to adopt this if you're touching RISCV regalloc?

Possibly. Let's chat about it when we talk Wednesday. I'm missing a bit of context on motivation, and mechanism.

preames · 2025-03-20T15:08:59Z

@preames want to adopt this if you're touching RISCV regalloc?

Possibly. Let's chat about it when we talk Wednesday. I'm missing a bit of context on motivation, and mechanism.

I talked with @arsenm and I am going to adopt this one. The motivation here is in optimizing spill/fill placement after initial spilling via the postOptimzation hooks. Expect to see a patch from me in this area in the next week or so.

Motivation is supporting scalable spills and reloads, e.g. in llvm#120524. Looking at this API, I'm suspicious that the access size should just be coming from the memory operand on the load or store, but we don't appear to be consistently setting that up. That's a larger change so I may or may not bother pursuing that.

#132244) Motivation is supporting scalable spills and reloads, e.g. in #120524. Looking at this API, I'm suspicious that the access size should just be coming from the memory operand on the load or store, but we don't appear to be consistently setting that up. That's a larger change so I may or may not bother pursuing that.

…ckSlot [nfc] (#132244) Motivation is supporting scalable spills and reloads, e.g. in llvm/llvm-project#120524. Looking at this API, I'm suspicious that the access size should just be coming from the memory operand on the load or store, but we don't appear to be consistently setting that up. That's a larger change so I may or may not bother pursuing that.

@arsenm

This is an adapted version of arsenm's llvm#120524. The intention of the change is to enable dead stack slot copy elimination in StackSlotColoring for vector loads and stores. In terms of testing, see stack-slot-coloring.mir. This has little impact on in tree tests otherwise. This change has a different and smaller set of test diffs then then @arsenm's patch because I'm using scalable sizes for the LMULs, not a single signal value. His patch allowed vector load/store pairs of different width to be deleted, mine does not. There's also simply been a lot of churn in regalloc behavior on these particular tests recently, so that may explain some of the diff as well.

@arsenm

…ill (#132296) This is an adapted version of arsenm's #120524. The intention of the change is to enable dead stack slot copy elimination in StackSlotColoring for vector loads and stores. In terms of testing, see stack-slot-coloring.mir. This has little impact on in tree tests otherwise. This change has a different and smaller set of test diffs then then @arsenm's patch because I'm using scalable sizes for the LMULs, not a single signal value. His patch allowed vector load/store pairs of different width to be deleted, mine does not. There's also simply been a lot of churn in regalloc behavior on these particular tests recently, so that may explain some of the diff as well.

@arsenm

…tor spill/fill (#132296) This is an adapted version of arsenm's llvm/llvm-project#120524. The intention of the change is to enable dead stack slot copy elimination in StackSlotColoring for vector loads and stores. In terms of testing, see stack-slot-coloring.mir. This has little impact on in tree tests otherwise. This change has a different and smaller set of test diffs then then @arsenm's patch because I'm using scalable sizes for the LMULs, not a single signal value. His patch allowed vector load/store pairs of different width to be deleted, mine does not. There's also simply been a lot of churn in regalloc behavior on these particular tests recently, so that may explain some of the diff as well.

preames · 2025-03-21T17:53:21Z

My adopted version of this (#132296), has landed. I went back to check if the differences in MemBytes setup were material, and the test changes from this change on tip of tree are the same as my landed change. So, to my knowledge, no further work needed here.

arsenm added the backend:RISC-V label Dec 19, 2024 — with Graphite App

arsenm requested review from asb, lukel97, preames and topperc December 19, 2024 05:33

arsenm marked this pull request as ready for review December 19, 2024 05:33

wangpc-pp reviewed Dec 19, 2024

View reviewed changes

kito-cheng requested a review from BeMg January 7, 2025 06:49

topperc reviewed Jan 7, 2025

View reviewed changes

arsenm mentioned this pull request Feb 5, 2025

PeepholeOpt: Fix looking for def of current copy to coalesce #125533

Merged

arsenm added 2 commits February 13, 2025 20:28

RISCV: Implement isLoadFromStackSlot/isStoreToStackSlot for rvv

022fe3b

This partially helps avoid regressions in a future regalloc patch. It isn't sufficient, and I think there are more missing implementations of the copy and spill hooks.

Add a few more opcodes found by asserting on frame indexes with mayLo…

4e4e721

…ad/mayStore. One test looks like a regression, which is probably rematerialization not being handled for something.

arsenm force-pushed the users/arsenm/riscv-is-load-store-stackslot-rvv branch from 6b9fb88 to 4e4e721 Compare February 13, 2025 13:32

arsenm commented Feb 18, 2025

View reviewed changes

preames mentioned this pull request Mar 20, 2025

[TTI] Use TypeSize in isLoadFromStackSlot and isStoreToStackSlot [nfc] #132244

Merged

preames mentioned this pull request Mar 20, 2025

[RISCV] isLoadFromStackSlot and isStoreToStackSlot for vector spill/fill #132296

Merged

preames closed this Mar 21, 2025

arsenm deleted the users/arsenm/riscv-is-load-store-stackslot-rvv branch March 22, 2025 11:48

RISCV: Implement isLoadFromStackSlot/isStoreToStackSlot for rvv #120524

RISCV: Implement isLoadFromStackSlot/isStoreToStackSlot for rvv #120524

Uh oh!

Conversation

arsenm commented Dec 19, 2024

Uh oh!

arsenm commented Dec 19, 2024

Uh oh!

llvmbot commented Dec 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangpc-pp commented Dec 19, 2024

Uh oh!

arsenm commented Dec 19, 2024

Uh oh!

arsenm commented Jan 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

arsenm commented Mar 14, 2025

Uh oh!

preames commented Mar 17, 2025

Uh oh!

preames commented Mar 20, 2025

Uh oh!

preames commented Mar 21, 2025

Uh oh!

Uh oh!