[RISCV] Add OR/XOR/SUB to RISCVInstrInfo::isCopyInstrImpl

[asb]

This adds coverage for additional instructions in isCopyInstrImpl, for now picking just those where I can observe that there is a codegen difference for SPEC.

---

Implementing these removes a small number of effectively no-op instructions in a SPEC compilation. As a starting point I've just added the cases that cause a code gen difference, but welcome discussion on to what degree we want isCopyInstrImpl to be exhaustive.

I would highlight:

• The case of OR with indentical register operands isn't covered. Although this causes no codegen difference, possibly I should update this PR to handle it? That way at least the opcodes that isCopyInstrImpl recognises are covered exhaustively.
• Other instructions tried that have no impact on SPEC codegen:
  • ORI/XORI with imm == 0
  • ANDN with second operand == X0
  • ROL/ROR with X0 rotate value
  • MIN/MINU/MAX/MAXU with equal operands

[llvmbot]

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

Author: Alex Bradbury (asb)

Changes

This adds coverage for additional instructions in isCopyInstrImpl, for now picking just those where I can observe that there is a codegen difference for SPEC.

---

Implementing these removes a small number of effectively no-op instructions in a SPEC compilation. As a starting point I've just added the cases that cause a code gen difference, but welcome discussion on to what degree we want isCopyInstrImpl to be exhaustive.

I would highlight:

• The case of OR with indentical register operands isn't covered. Although this causes no codegen difference, possibly I should update this PR to handle it? That way at least the opcodes that isCopyInstrImpl recognises are covered exhaustively.
• Other instructions tried that have no impact on SPEC codegen:
  • ORI/XORI with imm == 0
  • ANDN with second operand == X0
  • ROL/ROR with X0 rotate value
  • MIN/MINU/MAX/MAXU with equal operands

---

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

2 Files Affected:

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

diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
index c197f06855b6e..84e167e452130 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
@@ -1660,6 +1660,8 @@ RISCVInstrInfo::isCopyInstrImpl(const MachineInstr &MI) const {
   default:
     break;
   case RISCV::ADD:
+  case RISCV::OR:
+  case RISCV::XOR:
     if (MI.getOperand(1).isReg() && MI.getOperand(1).getReg() == RISCV::X0 &&
         MI.getOperand(2).isReg())
       return DestSourcePair{MI.getOperand(0), MI.getOperand(2)};
@@ -1673,6 +1675,11 @@ RISCVInstrInfo::isCopyInstrImpl(const MachineInstr &MI) const {
         MI.getOperand(2).getImm() == 0)
       return DestSourcePair{MI.getOperand(0), MI.getOperand(1)};
     break;
+  case RISCV::SUB:
+    if (MI.getOperand(2).isReg() && MI.getOperand(2).getReg() == RISCV::X0 &&
+        MI.getOperand(1).isReg())
+      return DestSourcePair{MI.getOperand(0), MI.getOperand(1)};
+    break;
   case RISCV::FSGNJ_D:
   case RISCV::FSGNJ_S:
   case RISCV::FSGNJ_H:
diff --git a/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp b/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp
index f7351f00ae282..ac802c8d33fa1 100644
--- a/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp
+++ b/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp
@@ -135,31 +135,50 @@ TEST_P(RISCVInstrInfoTest, IsCopyInstrImpl) {
   EXPECT_EQ(MI4Res->Destination->getReg(), RISCV::F1_D);
   EXPECT_EQ(MI4Res->Source->getReg(), RISCV::F2_D);
 
-  // ADD.
-  MachineInstr *MI5 = BuildMI(*MF, DL, TII->get(RISCV::ADD), RISCV::X1)
-                          .addReg(RISCV::X2)
-                          .addReg(RISCV::X3)
-                          .getInstr();
-  auto MI5Res = TII->isCopyInstrImpl(*MI5);
-  EXPECT_FALSE(MI5Res.has_value());
+  // ADD/OR/XOR.
+  for (unsigned Opc : {RISCV::ADD, RISCV::OR, RISCV::XOR}) {
+    MachineInstr *MI5 = BuildMI(*MF, DL, TII->get(Opc), RISCV::X1)
+                            .addReg(RISCV::X2)
+                            .addReg(RISCV::X3)
+                            .getInstr();
+    auto MI5Res = TII->isCopyInstrImpl(*MI5);
+    EXPECT_FALSE(MI5Res.has_value());
+
+    MachineInstr *MI6 = BuildMI(*MF, DL, TII->get(Opc), RISCV::X1)
+                            .addReg(RISCV::X0)
+                            .addReg(RISCV::X2)
+                            .getInstr();
+    auto MI6Res = TII->isCopyInstrImpl(*MI6);
+    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(Opc), RISCV::X1)
+                            .addReg(RISCV::X2)
+                            .addReg(RISCV::X0)
+                            .getInstr();
+    auto MI7Res = TII->isCopyInstrImpl(*MI7);
+    ASSERT_TRUE(MI7Res.has_value());
+    EXPECT_EQ(MI7Res->Destination->getReg(), RISCV::X1);
+    EXPECT_EQ(MI7Res->Source->getReg(), RISCV::X2);
+  }
 
-  MachineInstr *MI6 = BuildMI(*MF, DL, TII->get(RISCV::ADD), RISCV::X1)
+  // SUB.
+  MachineInstr *MI8 = BuildMI(*MF, DL, TII->get(RISCV::SUB), RISCV::X1)
                           .addReg(RISCV::X0)
                           .addReg(RISCV::X2)
                           .getInstr();
-  auto MI6Res = TII->isCopyInstrImpl(*MI6);
-  ASSERT_TRUE(MI6Res.has_value());
-  EXPECT_EQ(MI6Res->Destination->getReg(), RISCV::X1);
-  EXPECT_EQ(MI6Res->Source->getReg(), RISCV::X2);
+  auto MI8Res = TII->isCopyInstrImpl(*MI8);
+  EXPECT_FALSE(MI8Res.has_value());
 
-  MachineInstr *MI7 = BuildMI(*MF, DL, TII->get(RISCV::ADD), RISCV::X1)
+  MachineInstr *MI9 = BuildMI(*MF, DL, TII->get(RISCV::SUB), RISCV::X1)
                           .addReg(RISCV::X2)
                           .addReg(RISCV::X0)
                           .getInstr();
-  auto MI7Res = TII->isCopyInstrImpl(*MI7);
-  ASSERT_TRUE(MI7Res.has_value());
-  EXPECT_EQ(MI7Res->Destination->getReg(), RISCV::X1);
-  EXPECT_EQ(MI7Res->Source->getReg(), RISCV::X2);
+  auto MI9Res = TII->isCopyInstrImpl(*MI9);
+  ASSERT_TRUE(MI9Res.has_value());
+  EXPECT_EQ(MI9Res->Destination->getReg(), RISCV::X1);
+  EXPECT_EQ(MI9Res->Source->getReg(), RISCV::X2);
 }
 
 TEST_P(RISCVInstrInfoTest, GetMemOperandsWithOffsetWidth) {

[topperc]

LGTM

[mshockwave]

LGTM. Since you mentioned rotate with zero offset, have you seen any left/right shiftings with zero offset in SPEC?

[mikhailramalho]

LGTM

[preames]

welcome discussion on to what degree we want isCopyInstrImpl to be exhaustive.

In general, sticking to those that we can write a test case from IR (or otherwise not over reduced) for seems like a reasonable line. That's not quite the same as causing codegen differences in spec, but if we can't test something, having code seems undesirable. If we can test something, but it doesn't show up, then I'd say that's largely at the discretion of the author. (i.e. if they find the time worthwhile)

* The case of OR with indentical register operands isn't covered. Although this causes no codegen difference, possibly I should update this PR to handle it? That way at least the opcodes that isCopyInstrImpl recognises are covered exhaustively.

This might be a reasonable exception to the "only with high level tests" policy I suggest above. I could go either way, and am happy to defer to your judgement.

[preames]

One aside here: The need to handle all these cases hints we have a canonicalization problem. All of these should be canonicalized into a single instruction. Unless having them in the isCopyInstrImpl enables that canonicalization - I haven't checked - we might have something to follow up on.

[asb]

I explored some of the things mentioned here in review:

• I don't see any left/right shifts with 0. I do however see left/right shifts of the zero register. I've got a pretty comprehensive dump of 'odd' scalar instructions in llvm-test-suite binaries (either deletable as nops, or an inefficient encoding or some other operation that should be replaced). I'll make a tracking issue summarising this.
• I won't add the or with equal reg args case for the time being. The script I have will flag if I start seeing it in the test suite / spec, and perhaps it becomes clear what coverage we want and where when tracking down other unwanted instructions.
• I don't believe this is a canonicalisation problem. I've sampled a few affected cases, made an llvm-reduced test case and looked at the log. I've confirmed it's the same flow that motivated the recent extension to MachineCopyPropagation to catch these nops. For one I just looked through the tail duplicator kicks in, one of the branches ends up with a COPY of x0 feeding into the sub while the other has a semantically sensible sub. MachineCopyPropagation is run just after it in the pass pipeline, and once we fill in these missing gaps it succeeds in its goal of cleaning up the tail duplicator output.

I'll go ahead and merge this now and follow-up on other observed oddities with PRs / issues.

[asb]

The promised tracking/summary issue is up at #133449