From 05077e9b5ebb88090d62a7b21da8ce70facc4431 Mon Sep 17 00:00:00 2001 From: Alex Bradbury Date: Thu, 8 Feb 2024 11:13:24 +0000 Subject: [PATCH 1/2] [RISCV] Handle ADD in RISCVInstrInfo::isCopyInstrImpl 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). --- llvm/lib/Target/RISCV/RISCVInstrInfo.cpp | 6 ++++++ llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp | 10 +++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp index 89eb71d917428..ce3f925cbc58a 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 5f3ce53f5d274..fab264c8e6b53 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) { From 20f59583b7deeb39efbceb64d6b62f4bb36cf901 Mon Sep 17 00:00:00 2001 From: Alex Bradbury Date: Thu, 8 Feb 2024 11:19:25 +0000 Subject: [PATCH 2/2] Reland [RISCV] Implement RISCVInsrInfo::getConstValDefinedInReg 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. --- llvm/lib/Target/RISCV/RISCVInstrInfo.cpp | 27 +++++++++++ llvm/lib/Target/RISCV/RISCVInstrInfo.h | 3 ++ .../Target/RISCV/RISCVInstrInfoTest.cpp | 46 +++++++++++++++++++ 3 files changed, 76 insertions(+) diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp index ce3f925cbc58a..4390f587a9415 100644 --- a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp +++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp @@ -2562,6 +2562,33 @@ std::optional 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 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 fab264c8e6b53..ae3cb78538fef 100644 --- a/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp +++ b/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp @@ -161,6 +161,52 @@ TEST_P(RISCVInstrInfoTest, IsCopyInstrImpl) { 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) { const RISCVInstrInfo *TII = ST->getInstrInfo(); const TargetRegisterInfo *TRI = ST->getRegisterInfo();