[RISCV] Handle ADD in RISCVInstrInfo::isCopyInstrImpl

[asb]

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).

[llvmbot]

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

Author: Alex Bradbury (asb)

Changes

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).

---

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

2 Files Affected:

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

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) {

[preames]

LGTM

[topperc]

Do we need to check that Operand 2 is a register?

[preames]

Isn't that implied by the instruction opcode? It can't be an immediate. Are you thinking a frame index or something like that?

[topperc]

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.