Skip to content

[RISCV] Implement RISCVInsrInfo::getConstValDefinedInReg #77610

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

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

asb
Copy link
Contributor

@asb asb commented Jan 10, 2024

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.

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
Member

llvmbot commented Jan 10, 2024

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

Author: Alex Bradbury (asb)

Changes

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/77610.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 (+46)
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..4b1754cd4fcdcf 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();

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

@asb asb merged commit 57d517c into llvm:main Jan 16, 2024
@rofirrim
Copy link
Collaborator

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?

asb added a commit that referenced this pull request Jan 17, 2024
This reverts commit 4b7d997.

A miscompile was reported
<#77610 (comment)>.
Reverting so it can be investigated.
@asb
Copy link
Contributor Author

asb commented Jan 17, 2024

@rofirrim thanks for the report. I won't be able to look until tomorrow so I've gone ahead and reverted until then. Looking again, it's very possible there's a sign extension related issue lurking.

ampandey-1995 pushed a commit to ampandey-1995/llvm-project that referenced this pull request Jan 19, 2024
This reverts commit 4b7d997.

A miscompile was reported
<llvm#77610 (comment)>.
Reverting so it can be investigated.
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
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.
asb added a commit to asb/llvm-project that referenced this pull request Feb 8, 2024
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).
asb added a commit to asb/llvm-project that referenced this pull request Feb 8, 2024
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.
asb added a commit that referenced this pull request Mar 5, 2025
Split out from #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).

This is also motivated by future changes to MachineCopyPropagation which will use this information to determine if we have been left with a nop mv.
jph-13 pushed a commit to jph-13/llvm-project that referenced this pull request Mar 21, 2025
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).

This is also motivated by future changes to MachineCopyPropagation which will use this information to determine if we have been left with a nop mv.
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.

4 participants