Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reland [RISCV] Implement RISCVInsrInfo::getConstValDefinedInReg #81124

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

asb
Copy link
Contributor

@asb asb commented Feb 8, 2024

Reland of #77610, with the previously buggy isCopyInstrImpl change split out to #81123.

This helper function handles common cases where we can determine a
constant value is being defined in a register. Although it looks like
codegen changes are possible due to this being called in
PeepholeOptimizer, my main motivation is to use this in
describeLoadedValue.

Split out from llvm#77610 and features a test, as a buggy version of this
caused a regression when landing that patch (the previous version had a
typo picking the wrong register as the source).
Reland of llvm#77610, with the previously buggy isCopyInstrImpl change split
out to llvm#81123.

This helper function handles common cases where we can determine a
constant value is being defined in a register. Although it looks like
codegen changes are possible due to this being called in
PeepholeOptimizer, my main motivation is to use this in
describeLoadedValue.
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 8, 2024

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

Author: Alex Bradbury (asb)

Changes

Reland of #77610, with the previously buggy isCopyInstrImpl change split out to #81123.

This helper function handles common cases where we can determine a
constant value is being defined in a register. Although it looks like
codegen changes are possible due to this being called in
PeepholeOptimizer, my main motivation is to use this in
describeLoadedValue.


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

3 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVInstrInfo.cpp (+33)
  • (modified) llvm/lib/Target/RISCV/RISCVInstrInfo.h (+3)
  • (modified) llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp (+53-3)
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
index 89eb71d917428..4390f587a9415 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
@@ -1579,6 +1579,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(2)};
+    if (MI.getOperand(2).isReg() && MI.getOperand(2).getReg() == RISCV::X0)
+      return DestSourcePair{MI.getOperand(0), MI.getOperand(1)};
+    break;
   case RISCV::ADDI:
     // Operand 1 can be a frameindex but callers expect registers
     if (MI.getOperand(1).isReg() && MI.getOperand(2).isImm() &&
@@ -2556,6 +2562,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 0f7d3e4e43390..5d20276696aea 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 5f3ce53f5d274..ae3cb78538fef 100644
--- a/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp
+++ b/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp
@@ -134,7 +134,7 @@ TEST_P(RISCVInstrInfoTest, IsCopyInstrImpl) {
   EXPECT_EQ(MI4Res->Destination->getReg(), RISCV::F1_D);
   EXPECT_EQ(MI4Res->Source->getReg(), RISCV::F2_D);
 
-  // ADD. TODO: Should return true for add reg, x0 and add x0, reg.
+  // ADD.
   MachineInstr *MI5 = BuildMI(*MF, DL, TII->get(RISCV::ADD), RISCV::X1)
                           .addReg(RISCV::X2)
                           .addReg(RISCV::X3)
@@ -147,14 +147,64 @@ TEST_P(RISCVInstrInfoTest, IsCopyInstrImpl) {
                           .addReg(RISCV::X2)
                           .getInstr();
   auto MI6Res = TII->isCopyInstrImpl(*MI6);
-  EXPECT_FALSE(MI6Res.has_value());
+  ASSERT_TRUE(MI6Res.has_value());
+  EXPECT_EQ(MI6Res->Destination->getReg(), RISCV::X1);
+  EXPECT_EQ(MI6Res->Source->getReg(), RISCV::X2);
 
   MachineInstr *MI7 = BuildMI(*MF, DL, TII->get(RISCV::ADD), RISCV::X1)
                           .addReg(RISCV::X2)
                           .addReg(RISCV::X0)
                           .getInstr();
   auto MI7Res = TII->isCopyInstrImpl(*MI7);
-  EXPECT_FALSE(MI7Res.has_value());
+  ASSERT_TRUE(MI7Res.has_value());
+  EXPECT_EQ(MI7Res->Destination->getReg(), RISCV::X1);
+  EXPECT_EQ(MI7Res->Source->getReg(), RISCV::X2);
+}
+
+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) {

Copy link
Collaborator

@preames preames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM w/optional follow up idea.

return true;
}

if (!(MI.getOpcode() == RISCV::ADDI || MI.getOpcode() == RISCV::ADDIW ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reuse TII's ::isAddImmediate here? It only covers the ADDI case, but we could land that, and then extend it to cover the other cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants