[RISCV] Support FrameIndex operands in getMemOperandsWithOffsetWidth / getMemOperandWithOffsetWidth

[asb]

I noted AArch64 happily accepts a FrameIndex operand as well as a register. This doesn't cause any changes outside of my C++ unit test for the current state of in-tree, but this will cause additional test changes if #73789 is rebased on top of it.

---

I do have some caution here that the returned Offset doesn't seem at all as meaningful if you have a FrameIndex base. Though as I say, AArch64 has been doing this for some time - see https://reviews.llvm.org/D54847. I believe this change won't harm the approach taken in shouldClusterMemOps because memOpsHaveSameBasePtr will only return true if the FrameIndex operand is the same for both operations. (Though reviewers, please double-check you agree). This patch is directly applyable, so I've opted not to base it on top of #73789.

[llvmbot]

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

Author: Alex Bradbury (asb)

Changes

I noted AArch64 happily accepts a FrameIndex operand as well as a register. This doesn't cause any changes outside of my C++ unit test for the current state of in-tree, but this will cause additional test changes if #73789 is rebased on top of it.

---

I do have some caution here that the returned Offset doesn't seem at all as meaningful if you have a FrameIndex base. Though as I say, AArch64 has been doing this for some time - see https://reviews.llvm.org/D54847. I believe this change won't harm the approach taken in shouldClusterMemOps because memOpsHaveSameBasePtr will only return true if the FrameIndex operand is the same for both operations. (Though reviewers, please double-check you agree). This patch is directly applyable, so I've opted not to base it on top of #73789.

---

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

2 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVInstrInfo.cpp (+1-1)
• (modified) llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp (+7-2)

diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
index 2918e5654db4f9f..900eede0c4ef24c 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
@@ -2304,7 +2304,7 @@ bool RISCVInstrInfo::getMemOperandWithOffsetWidth(
   // load/store instructions.
   if (LdSt.getNumExplicitOperands() != 3)
     return false;
-  if (!LdSt.getOperand(1).isReg() || !LdSt.getOperand(2).isImm())
+  if ((!LdSt.getOperand(1).isReg() && !LdSt.getOperand(1).isFI()) || !LdSt.getOperand(2).isImm())
     return false;
 
   if (!LdSt.hasOneMemOperand())
diff --git a/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp b/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp
index 135d7dbb426e3c2..b4c96a9c2a62ce7 100644
--- a/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp
+++ b/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp
@@ -154,7 +154,6 @@ TEST_P(RISCVInstrInfoTest, GetMemOperandsWithOffsetWidth) {
   Res = TII->getMemOperandsWithOffsetWidth(*MI, BaseOps, Offset,
                                            OffsetIsScalable, Width, TRI);
 
-  // TODO: AArch64 can handle this case, and we probably should too.
   BaseOps.clear();
   MMO = MF->getMachineMemOperand(MachinePointerInfo(),
                                  MachineMemOperand::MOStore, 4, Align(4));
@@ -165,7 +164,13 @@ TEST_P(RISCVInstrInfoTest, GetMemOperandsWithOffsetWidth) {
            .addMemOperand(MMO);
   Res = TII->getMemOperandsWithOffsetWidth(*MI, BaseOps, Offset,
                                            OffsetIsScalable, Width, TRI);
-  EXPECT_FALSE(Res);
+  ASSERT_TRUE(Res);
+  ASSERT_EQ(BaseOps.size(), 1u);
+  ASSERT_TRUE(BaseOps.front()->isFI());
+  EXPECT_EQ(BaseOps.front()->getIndex(), 2);
+  EXPECT_EQ(Offset, 4);
+  EXPECT_FALSE(OffsetIsScalable);
+  EXPECT_EQ(Width, 4u);
 }
 
 } // namespace

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[topperc]

LGTM

[wangpc-pp]

LGTM.