[RISCV] Implement RISCVInstrInfo::isAddImmediate #72356
Conversation
This hook is called by the target-independent implementation of TargetInstrInfo::describeLoadedValue. I've opted to test it via a C++ unit test, which although fiddly to set up seems the right way to test a function with such clear intended semantics (rather than testing the impact indirectly). isAddImmediate will never recognise ADDIW as an add immediate which I _think_ is conservatively correct, as the caller may not understand its semantics vs ADDI. Note that although the doc comment for isAddImmediate specifies its behaviuor solely in terms of physical registers, none of the current in-tree implementations (including this one) bail out on virtual registers.
|
@llvm/pr-subscribers-backend-risc-v Author: Alex Bradbury (asb) ChangesThis hook is called by the target-independent implementation of TargetInstrInfo::describeLoadedValue. I've opted to test it via a C++ unit test, which although fiddly to set up seems the right way to test a function with such clear intended semantics (rather than testing the impact indirectly). isAddImmediate will never recognise ADDIW as an add immediate which I think is conservatively correct, as the caller may not understand its semantics vs ADDI. Note that although the doc comment for isAddImmediate specifies its behaviuor solely in terms of physical registers, none of the current in-tree implementations (including this one) bail out on virtual registers. Full diff: https://github.com/llvm/llvm-project/pull/72356.diff 4 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
index 9271f807a84838b..45df0b111752c21 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
@@ -2438,6 +2438,23 @@ MachineBasicBlock::iterator RISCVInstrInfo::insertOutlinedCall(
return It;
}
+std::optional<RegImmPair> RISCVInstrInfo::isAddImmediate(const MachineInstr &MI,
+ Register Reg) const {
+ // TODO: Handle cases where Reg is a super- or sub-register of the
+ // destination register.
+ const MachineOperand &Op0 = MI.getOperand(0);
+ if (!Op0.isReg() || Reg != Op0.getReg())
+ return std::nullopt;
+
+ // Don't consider ADDIW as a candidate because the caller may not be aware
+ // of its sign extension behaviour.
+ if (MI.getOpcode() == RISCV::ADDI && MI.getOperand(1).isReg() &&
+ MI.getOperand(2).isImm())
+ return RegImmPair{MI.getOperand(1).getReg(), MI.getOperand(2).getImm()};
+
+ return std::nullopt;
+}
+
// 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 b33d8c28561596b..c2bffe1f9d6a1bf 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.h
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.h
@@ -197,6 +197,9 @@ class RISCVInstrInfo : public RISCVGenInstrInfo {
MachineBasicBlock::iterator &It, MachineFunction &MF,
outliner::Candidate &C) const override;
+ std::optional<RegImmPair> isAddImmediate(const MachineInstr &MI,
+ Register Reg) 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/CMakeLists.txt b/llvm/unittests/Target/RISCV/CMakeLists.txt
index 9d0bf7244c02210..d80d29b7f0dadac 100644
--- a/llvm/unittests/Target/RISCV/CMakeLists.txt
+++ b/llvm/unittests/Target/RISCV/CMakeLists.txt
@@ -7,13 +7,17 @@ set(LLVM_LINK_COMPONENTS
RISCVCodeGen
RISCVDesc
RISCVInfo
- TargetParser
+ CodeGen
+ Core
MC
+ SelectionDAG
+ TargetParser
)
add_llvm_target_unittest(RISCVTests
MCInstrAnalysisTest.cpp
RISCVBaseInfoTest.cpp
+ RISCVInstrInfoTest.cpp
)
set_property(TARGET RISCVTests PROPERTY FOLDER "Tests/UnitTests/TargetTests")
diff --git a/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp b/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp
new file mode 100644
index 000000000000000..1e9a259a6b3f110
--- /dev/null
+++ b/llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cpp
@@ -0,0 +1,96 @@
+//===- RISCVInstrInfoTest.cpp - RISCVInstrInfo unit tests -----------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "RISCVInstrInfo.h"
+#include "RISCVSubtarget.h"
+#include "RISCVTargetMachine.h"
+#include "llvm/CodeGen/MachineModuleInfo.h"
+#include "llvm/MC/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
+#include "llvm/Target/TargetMachine.h"
+#include "llvm/Target/TargetOptions.h"
+
+#include "gtest/gtest.h"
+
+#include <memory>
+
+using namespace llvm;
+
+namespace {
+
+class RISCVInstrInfoTest : public testing::TestWithParam<const char *> {
+protected:
+ std::unique_ptr<LLVMContext> Ctx;
+ std::unique_ptr<RISCVSubtarget> ST;
+ std::unique_ptr<MachineModuleInfo> MMI;
+ std::unique_ptr<MachineFunction> MF;
+
+ static void SetUpTestSuite() {
+ LLVMInitializeRISCVTargetInfo();
+ LLVMInitializeRISCVTarget();
+ LLVMInitializeRISCVTargetMC();
+ }
+
+ RISCVInstrInfoTest() {
+ std::string Error;
+ auto TT(Triple::normalize(GetParam()));
+ const Target *TheTarget = TargetRegistry::lookupTarget(TT, Error);
+ TargetOptions Options;
+
+ RISCVTargetMachine *TM = static_cast<RISCVTargetMachine *>(
+ TheTarget->createTargetMachine(TT, "generic", "", Options, std::nullopt,
+ std::nullopt, CodeGenOptLevel::Default));
+
+ Ctx = std::make_unique<LLVMContext>();
+ Module M("Module", *Ctx);
+ M.setDataLayout(TM->createDataLayout());
+ auto *FType = FunctionType::get(Type::getVoidTy(*Ctx), false);
+ auto *F = Function::Create(FType, GlobalValue::ExternalLinkage, "Test", &M);
+ MMI = std::make_unique<MachineModuleInfo>(TM);
+
+ ST = std::make_unique<RISCVSubtarget>(
+ TM->getTargetTriple(), TM->getTargetCPU(), TM->getTargetCPU(),
+ TM->getTargetFeatureString(),
+ TM->getTargetTriple().isArch64Bit() ? "lp64" : "ilp32", 0, 0, *TM);
+
+ MF = std::make_unique<MachineFunction>(*F, *TM, *ST, 42, *MMI);
+ }
+};
+
+TEST_P(RISCVInstrInfoTest, IsAddImmediate) {
+ const RISCVInstrInfo *TII = ST->getInstrInfo();
+ DebugLoc DL;
+
+ MachineInstr *MI1 = BuildMI(*MF, DL, TII->get(RISCV::ADDI), RISCV::X1)
+ .addReg(RISCV::X2)
+ .addImm(-128)
+ .getInstr();
+ auto MI1Res = TII->isAddImmediate(*MI1, RISCV::X1);
+ ASSERT_TRUE(MI1Res.has_value());
+ EXPECT_EQ(MI1Res->Reg, RISCV::X2);
+ EXPECT_EQ(MI1Res->Imm, -128);
+ EXPECT_FALSE(TII->isAddImmediate(*MI1, RISCV::X2).has_value());
+
+ MachineInstr *MI2 =
+ BuildMI(*MF, DL, TII->get(RISCV::LUI), RISCV::X1).addImm(-128).getInstr();
+ EXPECT_FALSE(TII->isAddImmediate(*MI2, RISCV::X1));
+
+ // Check ADDIW isn't treated as isAddImmediate.
+ if (ST->is64Bit()) {
+ MachineInstr *MI3 = BuildMI(*MF, DL, TII->get(RISCV::ADDIW), RISCV::X1)
+ .addReg(RISCV::X2)
+ .addImm(-128)
+ .getInstr();
+ EXPECT_FALSE(TII->isAddImmediate(*MI3, RISCV::X1));
+ }
+}
+
+} // namespace
+
+INSTANTIATE_TEST_SUITE_P(RV32And64, RISCVInstrInfoTest,
+ testing::Values("riscv32", "riscv64"));
|
I've posted a doc change PR #72357 to adjust this (or for someone to tell me why that's the wrong thing to do!). |
|
|
||
| // Don't consider ADDIW as a candidate because the caller may not be aware | ||
| // of its sign extension behaviour. | ||
| if (MI.getOpcode() == RISCV::ADDI && MI.getOperand(1).isReg() && |
There was a problem hiding this comment.
What circumstance that the third operand of ADDI is not an immediate?
There was a problem hiding this comment.
I'm not sure if we have such an instance. The AArch64 implementation is concerned that the third operand might be a global address but I haven't confirmed if that could happen for us. It just seemed sensible to code defensively.
There was a problem hiding this comment.
Do we need to handle the LI case like Mips does in MipsInstrInfo::describeLoadedValue? I'm not sure returning X0 as the register is meaningful to the caller of isAddImmediate.
There was a problem hiding this comment.
We do. I was hoping to land this change and then follow up with testing and improvements for describeLoadedValue. I'm not really sure whether landing this patch as-is would be considered to regress current behaviour or not. isAddImmediate was always returning false before so we weren't generating the DIExpression for these cases before, and even without the Mips special casing the handling of LI doesn't seem incorrect to my untrained eye (it's just that the reg+imm could be more efficiently represented as just imm in the case that reg==x0). I'm not a debuginfo expert so maybe there is a requirement that such expressions are simplified?
I think the LI case would be better handled in a target-independent way by having describeLoadedValue use getConstValDefinedInReg before then trying isAddImmediate. I'll take a look at how disruptive that change might be for other targets.
There was a problem hiding this comment.
Ok thanks. I'm not debuginfo expert either. So this patch LGTM
|
This appears to be breaking sanitizers: |
Since it's late for Alex, I'm working on a patch. |
Thanks for that Craig, really appreciate it. |
This hook is called by the target-independent implementation of TargetInstrInfo::describeLoadedValue. I've opted to test it via a C++ unit test, which although fiddly to set up seems the right way to test a function with such clear intended semantics (rather than testing the impact indirectly). isAddImmediate will never recognise ADDIW as an add immediate which I _think_ is conservatively correct, as the caller may not understand its semantics vs ADDI. Note that although the doc comment for isAddImmediate specifies its behaviour solely in terms of physical registers, none of the current in-tree implementations (including this one) bail out on virtual registers (see llvm#72357).
This hook is called by the target-independent implementation of TargetInstrInfo::describeLoadedValue. I've opted to test it via a C++ unit test, which although fiddly to set up seems the right way to test a function with such clear intended semantics (rather than testing the impact indirectly). isAddImmediate will never recognise ADDIW as an add immediate which I _think_ is conservatively correct, as the caller may not understand its semantics vs ADDI. Note that although the doc comment for isAddImmediate specifies its behaviour solely in terms of physical registers, none of the current in-tree implementations (including this one) bail out on virtual registers (see llvm#72357).
| std::nullopt, CodeGenOptLevel::Default)); | ||
|
|
||
| Ctx = std::make_unique<LLVMContext>(); | ||
| Module M("Module", *Ctx); |
There was a problem hiding this comment.
The module is destroyed at the end of the constructor.
As pointed out by @s-barannikov in post-commit review of #72356, the module is destroyed at the end of the constructor.
Tests are in preparation for adding handling of the load of a constant value as Mips does (noted in <llvm#72356 (comment)>). I've opted to implement these tests as a C++ unit test as on balance I _think_ it's easier to follow and maintain than .mir tests trying to indirectly test this function. That said, you see the limitations with the test of describeLoadedValue on a memory operation where we'd rather pass `MachinePointerInfo::getFixedStack` but can't because we'd need to then ensure the necessary stack metadata for the function is present.
Tests are in preparation for adding handling of the load of a constant value as Mips does (noted in <#72356 (comment)>). I've opted to implement these tests as a C++ unit test as on balance I _think_ it's easier to follow and maintain than .mir tests trying to indirectly test this function. That said, you see the limitations with the test of describeLoadedValue on a memory operation where we'd rather pass `MachinePointerInfo::getFixedStack` but can't because we'd need to then ensure the necessary stack metadata for the function is present.
This hook is called by the target-independent implementation of TargetInstrInfo::describeLoadedValue. I've opted to test it via a C++ unit test, which although fiddly to set up seems the right way to test a function with such clear intended semantics (rather than testing the impact indirectly).
isAddImmediate will never recognise ADDIW as an add immediate which I think is conservatively correct, as the caller may not understand its semantics vs ADDI.
Note that although the doc comment for isAddImmediate specifies its behaviuor solely in terms of physical registers, none of the current in-tree implementations (including this one) bail out on virtual registers.