[CodeGen][RISCV] Make default describeLoadedValue implementation call getConstValDefinedInReg

[asb]

This handles the same case that's supported in MipsInstrInfo::describeLoadedValue - allowing a simplified dwarf expression to be produced for when a register is set to a known constant value.

[llvmbot]

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

Author: Alex Bradbury (asb)

Changes

This handles the same case that's supported in MipsInstrInfo::describeLoadedValue - allowing a simplified dwarf expression to be produced for when a register is set to a known constant value.

Stacks on top of #77610

---

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

4 Files Affected:

• (modified) llvm/lib/CodeGen/TargetInstrInfo.cpp (+6)
• (modified) llvm/lib/Target/RISCV/RISCVInstrInfo.cpp (+33)
• (modified) llvm/lib/Target/RISCV/RISCVInstrInfo.h (+3)
• (modified) llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp (+49-5)

diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp
index 4783742a14ad7d..43edbc08e9039f 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -1499,6 +1499,12 @@ TargetInstrInfo::describeLoadedValue(const MachineInstr &MI,
   assert(MF->getProperties().hasProperty(
       MachineFunctionProperties::Property::NoVRegs));
 
+  int64_t ImmVal;
+  // A simplified DIExpression can be produced if the register is being set to
+  // a known constant value.
+  if (getConstValDefinedInReg(MI, Reg, ImmVal))
+    return ParamLoadedValue(MachineOperand::CreateImm(ImmVal), Expr);
+
   if (auto DestSrc = isCopyInstr(MI)) {
     Register DestReg = DestSrc->Destination->getReg();
 
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
index 9813c7a70dfc31..857e8979762cdc 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
@@ -1580,6 +1580,12 @@ RISCVInstrInfo::isCopyInstrImpl(const MachineInstr &MI) const {
   switch (MI.getOpcode()) {
   default:
     break;
+  case RISCV::ADD:
+    if (MI.getOperand(1).isReg() && MI.getOperand(1).getReg() == RISCV::X0)
+      return DestSourcePair{MI.getOperand(0), MI.getOperand(1)};
+    if (MI.getOperand(2).isReg() && MI.getOperand(2).getReg() == RISCV::X0)
+      return DestSourcePair{MI.getOperand(0), MI.getOperand(2)};
+    break;
   case RISCV::ADDI:
     // Operand 1 can be a frameindex but callers expect registers
     if (MI.getOperand(1).isReg() && MI.getOperand(2).isImm() &&
@@ -2555,6 +2561,33 @@ std::optional<RegImmPair> RISCVInstrInfo::isAddImmediate(const MachineInstr &MI,
   return std::nullopt;
 }
 
+bool RISCVInstrInfo::getConstValDefinedInReg(const MachineInstr &MI,
+                                             const Register Reg,
+                                             int64_t &ImmVal) const {
+  // Handle moves of X0.
+  if (auto DestSrc = isCopyInstr(MI)) {
+    if (DestSrc->Source->getReg() != RISCV::X0)
+      return false;
+    const Register DstReg = DestSrc->Destination->getReg();
+    if (DstReg != Reg)
+      return false;
+    ImmVal = 0;
+    return true;
+  }
+
+  if (!(MI.getOpcode() == RISCV::ADDI || MI.getOpcode() == RISCV::ADDIW ||
+        MI.getOpcode() == RISCV::ORI))
+    return false;
+  if (MI.getOperand(0).getReg() != Reg)
+    return false;
+  if (!MI.getOperand(1).isReg() || MI.getOperand(1).getReg() != RISCV::X0)
+    return false;
+  if (!MI.getOperand(2).isImm())
+    return false;
+  ImmVal = MI.getOperand(2).getImm();
+  return true;
+}
+
 // MIR printer helper function to annotate Operands with a comment.
 std::string RISCVInstrInfo::createMIROperandComment(
     const MachineInstr &MI, const MachineOperand &Op, unsigned OpIdx,
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.h b/llvm/lib/Target/RISCV/RISCVInstrInfo.h
index 7e1d3f31180650..84015a66fb23a4 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.h
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.h
@@ -212,6 +212,9 @@ class RISCVInstrInfo : public RISCVGenInstrInfo {
   std::optional<RegImmPair> isAddImmediate(const MachineInstr &MI,
                                            Register Reg) const override;
 
+  bool getConstValDefinedInReg(const MachineInstr &MI, const Register Reg,
+                               int64_t &ImmVal) const override;
+
   bool findCommutedOpIndices(const MachineInstr &MI, unsigned &SrcOpIdx1,
                              unsigned &SrcOpIdx2) const override;
   MachineInstr *commuteInstructionImpl(MachineInstr &MI, bool NewMI,
diff --git a/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp b/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp
index 5836239bc56fd6..ca9040f17bf043 100644
--- a/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp
+++ b/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp
@@ -94,6 +94,52 @@ TEST_P(RISCVInstrInfoTest, IsAddImmediate) {
   }
 }
 
+TEST_P(RISCVInstrInfoTest, GetConstValDefinedInReg) {
+  const RISCVInstrInfo *TII = ST->getInstrInfo();
+  DebugLoc DL;
+  int64_t ImmVal;
+
+  auto *MI1 = BuildMI(*MF, DL, TII->get(RISCV::ADD), RISCV::X1)
+                  .addReg(RISCV::X2)
+                  .addReg(RISCV::X3)
+                  .getInstr();
+  EXPECT_FALSE(TII->getConstValDefinedInReg(*MI1, RISCV::X1, ImmVal));
+
+  auto *MI2 = BuildMI(*MF, DL, TII->get(RISCV::ADDI), RISCV::X1)
+                  .addReg(RISCV::X0)
+                  .addImm(-128)
+                  .getInstr();
+  EXPECT_FALSE(TII->getConstValDefinedInReg(*MI2, RISCV::X0, ImmVal));
+  ASSERT_TRUE(TII->getConstValDefinedInReg(*MI2, RISCV::X1, ImmVal));
+  EXPECT_EQ(ImmVal, -128);
+
+  auto *MI3 = BuildMI(*MF, DL, TII->get(RISCV::ORI), RISCV::X2)
+                  .addReg(RISCV::X0)
+                  .addImm(1024)
+                  .getInstr();
+  EXPECT_FALSE(TII->getConstValDefinedInReg(*MI3, RISCV::X0, ImmVal));
+  ASSERT_TRUE(TII->getConstValDefinedInReg(*MI3, RISCV::X2, ImmVal));
+  EXPECT_EQ(ImmVal, 1024);
+
+  if (ST->is64Bit()) {
+    auto *MI4 = BuildMI(*MF, DL, TII->get(RISCV::ADDIW), RISCV::X2)
+                    .addReg(RISCV::X0)
+                    .addImm(512)
+                    .getInstr();
+    EXPECT_FALSE(TII->getConstValDefinedInReg(*MI4, RISCV::X0, ImmVal));
+    ASSERT_TRUE(TII->getConstValDefinedInReg(*MI4, RISCV::X2, ImmVal));
+    EXPECT_EQ(ImmVal, 512);
+  }
+
+  auto *MI5 = BuildMI(*MF, DL, TII->get(RISCV::ADD), RISCV::X1)
+                  .addReg(RISCV::X0)
+                  .addReg(RISCV::X0)
+                  .getInstr();
+  EXPECT_FALSE(TII->getConstValDefinedInReg(*MI5, RISCV::X0, ImmVal));
+  ASSERT_TRUE(TII->getConstValDefinedInReg(*MI5, RISCV::X1, ImmVal));
+  EXPECT_EQ(ImmVal, 0);
+}
+
 TEST_P(RISCVInstrInfoTest, GetMemOperandsWithOffsetWidth) {
   const RISCVInstrInfo *TII = ST->getInstrInfo();
   const TargetRegisterInfo *TRI = ST->getRegisterInfo();
@@ -211,11 +257,9 @@ TEST_P(RISCVInstrInfoTest, DescribeLoadedValue) {
   std::optional<ParamLoadedValue> MI2Res =
       TII->describeLoadedValue(*MI2, RISCV::X3);
   ASSERT_TRUE(MI2Res.has_value());
-  ASSERT_TRUE(MI2Res->first.isReg());
-  EXPECT_EQ(MI2Res->first.getReg(), RISCV::X0);
-  // TODO: Could be a DW_OP_constu if this is recognised as a immediate load
-  // rather than just an addi.
-  expectDIEPrintResult(MI2Res->second, "!DIExpression(DW_OP_plus_uconst, 111)");
+  ASSERT_TRUE(MI2Res->first.isImm());
+  EXPECT_EQ(MI2Res->first.getImm(), 111);
+  expectDIEPrintResult(MI2Res->second, "!DIExpression()");
 
   // Add immediate.
   auto *MI3 = BuildMI(*MBB, MBB->begin(), DL, TII->get(RISCV::ADDI), RISCV::X2)

[asb]

Rebased now #77610 landed.

[rofirrim]

Hi @asb,

bisecting a failure on riscv64-unknown-linux-gnu in the llvm-testsuite showed that this change causes a failure on

Failed Tests (1):
  test-suite :: MultiSource/Benchmarks/MiBench/consumer-typeset/consumer-typeset.test

Can you reproduce it too, locally?

[rofirrim]

Hi @asb,

bisecting a failure on riscv64-unknown-linux-gnu in the llvm-testsuite showed that this change causes a failure on

Failed Tests (1):
  test-suite :: MultiSource/Benchmarks/MiBench/consumer-typeset/consumer-typeset.test

Can you reproduce it too, locally?

Sorry ignore me, bisect is pointing at #77610 and I got the wrong PR

[preames]

LGTM from the RISCV perspective. I'd appreciate someone more familiar with the dwarf ecosystem to chime in. I'm a bit worried we'll find missing support elsewhere in the toolchain if we start generating the new sub-expression type, but have no real way to quantify this risk. It's completely possible this is a non-issue.