[RISCV] Handle ADD in RISCVInstrInfo::isCopyInstrImpl #81123
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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).
|
@llvm/pr-subscribers-llvm-globalisel @llvm/pr-subscribers-backend-risc-v Author: Alex Bradbury (asb) ChangesSplit 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). Full diff: https://github.com/llvm/llvm-project/pull/81123.diff 2 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
index 89eb71d917428e..ce3f925cbc58a7 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() &&
diff --git a/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp b/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp
index 5f3ce53f5d274e..fab264c8e6b53c 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,18 @@ 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, GetMemOperandsWithOffsetWidth) {
|
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.
topperc
reviewed
Feb 8, 2024
| @@ -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)}; | |||
There was a problem hiding this comment.
Do we need to check that Operand 2 is a register?
There was a problem hiding this comment.
Isn't that implied by the instruction opcode? It can't be an immediate. Are you thinking a frame index or something like that?
There was a problem hiding this comment.
Yes, though that's probably impossible given current FrameIndex usage. But if we want to assume that, then the isReg check before checking X0 is unneeded. So we're not consistent.
There was a problem hiding this comment.
I've added the isReg checks for the other operand for consistency.
Not clear these are needed, but it is consistent.
|
I'd somehow missed that I'd never merged this after getting a positive review - apologies (it happens I was working on something else that needs this change). I've tested it on HEAD and added the extract isReg check Craig suggested. I will now merge. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/55/builds/7956 Here is the relevant piece of the build log for the reference |
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.
12 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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).