[llvm] [MCA] Parameterize variant scheduling classes by explicit variable (PR #92849)
Aiden Grossman via llvm-commits
llvm-commits at lists.llvm.org
Mon May 20 20:40:50 PDT 2024
https://github.com/boomanaiden154 created https://github.com/llvm/llvm-project/pull/92849
This patch looks up variant scheduling classes using an explicit variable (InstructionAddress passed to createInstruction) rather than simply using the address of the MCInst that gets passed into createInstruction. This breaks certain use cases that might occur out of tree, like decoding an execution trace instruction by instruction and creating MCA instructions as one goes along, like in the MCAD case. In this case, the MCInst will always have the same address and thus all instructions with the same variant scheduling class will end up with the same instruction description, leading to undesired behavior (assertions, uses after free, invalid results, etc.).
CC: @mtrofin
>From 37235b9927c3b60810b05100aa9381ec8b5febf9 Mon Sep 17 00:00:00 2001
From: Aiden Grossman <aidengrossman at google.com>
Date: Tue, 21 May 2024 03:36:53 +0000
Subject: [PATCH] [MCA] Parameterize variant scheduling classes by explicit
variable
This patch looks up variant scheduling classes using an explicit
variable (InstructionAddress passed to createInstruction) rather than
simply using the address of the MCInst that gets passed into
createInstruction. This breaks certain use cases that might occur out of
tree, like decoding an execution trace instruction by instruction and
creating MCA instructions as one goes along, like in the MCAD case. In
this case, the MCInst will always have the same address and thus all
instructions with the same variant scheduling class will end up with the
same instruction description, leading to undesired behavior (assertions,
uses after free, invalid results, etc.).
---
llvm/include/llvm/MCA/InstrBuilder.h | 18 +++++++++++++-----
llvm/lib/MCA/InstrBuilder.cpp | 18 +++++++++++-------
llvm/tools/llvm-mca/llvm-mca.cpp | 4 ++--
llvm/unittests/tools/llvm-mca/MCATestBase.cpp | 4 ++--
.../tools/llvm-mca/X86/TestIncrementalMCA.cpp | 4 ++--
5 files changed, 30 insertions(+), 18 deletions(-)
diff --git a/llvm/include/llvm/MCA/InstrBuilder.h b/llvm/include/llvm/MCA/InstrBuilder.h
index 3594372489148..67f9c736054e1 100644
--- a/llvm/include/llvm/MCA/InstrBuilder.h
+++ b/llvm/include/llvm/MCA/InstrBuilder.h
@@ -72,7 +72,7 @@ class InstrBuilder {
Descriptors;
// Key is the MCIInst and SchedClassID the describe the value InstrDesc
- DenseMap<std::pair<const MCInst *, unsigned>,
+ DenseMap<std::pair<uint64_t, unsigned>,
std::unique_ptr<const InstrDesc>>
VariantDescriptors;
@@ -83,10 +83,11 @@ class InstrBuilder {
InstRecycleCallback InstRecycleCB;
Expected<const InstrDesc &>
- createInstrDescImpl(const MCInst &MCI, const SmallVector<Instrument *> &IVec);
+ createInstrDescImpl(const MCInst &MCI, const SmallVector<Instrument *> &IVec,
+ uint64_t InstructionAddress);
Expected<const InstrDesc &>
- getOrCreateInstrDesc(const MCInst &MCI,
- const SmallVector<Instrument *> &IVec);
+ getOrCreateInstrDesc(const MCInst &MCI, const SmallVector<Instrument *> &IVec,
+ uint64_t InstructionAddress);
InstrBuilder(const InstrBuilder &) = delete;
InstrBuilder &operator=(const InstrBuilder &) = delete;
@@ -111,8 +112,15 @@ class InstrBuilder {
/// or null if there isn't any.
void setInstRecycleCallback(InstRecycleCallback CB) { InstRecycleCB = CB; }
+ /// Create an MCA Instruction from a MC Instruction that contains all the
+ /// relevant state MCA needs for modeling. Variant instructions (e.g.,
+ /// register zeroing idioms) each need their own instruction description
+ /// which is uniqued based on InstructionAddress. This can be an actual
+ /// address or something unique per instruction like a loop iteration
+ /// variable, but it must uniquely identify the instruction being passed in.
Expected<std::unique_ptr<Instruction>>
- createInstruction(const MCInst &MCI, const SmallVector<Instrument *> &IVec);
+ createInstruction(const MCInst &MCI, const SmallVector<Instrument *> &IVec,
+ uint64_t InstructionAddress);
};
} // namespace mca
} // namespace llvm
diff --git a/llvm/lib/MCA/InstrBuilder.cpp b/llvm/lib/MCA/InstrBuilder.cpp
index bcf065c566918..378b88812513f 100644
--- a/llvm/lib/MCA/InstrBuilder.cpp
+++ b/llvm/lib/MCA/InstrBuilder.cpp
@@ -521,7 +521,8 @@ Error InstrBuilder::verifyInstrDesc(const InstrDesc &ID,
Expected<const InstrDesc &>
InstrBuilder::createInstrDescImpl(const MCInst &MCI,
- const SmallVector<Instrument *> &IVec) {
+ const SmallVector<Instrument *> &IVec,
+ uint64_t InstructionAddress) {
assert(STI.getSchedModel().hasInstrSchedModel() &&
"Itineraries are not yet supported!");
@@ -603,14 +604,15 @@ InstrBuilder::createInstrDescImpl(const MCInst &MCI,
return *Descriptors[DKey];
}
- auto VDKey = std::make_pair(&MCI, SchedClassID);
+ auto VDKey = std::make_pair(InstructionAddress, SchedClassID);
VariantDescriptors[VDKey] = std::move(ID);
return *VariantDescriptors[VDKey];
}
Expected<const InstrDesc &>
InstrBuilder::getOrCreateInstrDesc(const MCInst &MCI,
- const SmallVector<Instrument *> &IVec) {
+ const SmallVector<Instrument *> &IVec,
+ uint64_t InstructionAddress) {
// Cache lookup using SchedClassID from Instrumentation
unsigned SchedClassID = IM.getSchedClassID(MCII, MCI, IVec);
@@ -620,19 +622,21 @@ InstrBuilder::getOrCreateInstrDesc(const MCInst &MCI,
unsigned CPUID = STI.getSchedModel().getProcessorID();
SchedClassID = STI.resolveVariantSchedClass(SchedClassID, &MCI, &MCII, CPUID);
- auto VDKey = std::make_pair(&MCI, SchedClassID);
+ auto VDKey = std::make_pair(InstructionAddress, SchedClassID);
if (VariantDescriptors.contains(VDKey))
return *VariantDescriptors[VDKey];
- return createInstrDescImpl(MCI, IVec);
+ return createInstrDescImpl(MCI, IVec, InstructionAddress);
}
STATISTIC(NumVariantInst, "Number of MCInsts that doesn't have static Desc");
Expected<std::unique_ptr<Instruction>>
InstrBuilder::createInstruction(const MCInst &MCI,
- const SmallVector<Instrument *> &IVec) {
- Expected<const InstrDesc &> DescOrErr = getOrCreateInstrDesc(MCI, IVec);
+ const SmallVector<Instrument *> &IVec,
+ uint64_t InstructionAddress) {
+ Expected<const InstrDesc &> DescOrErr =
+ getOrCreateInstrDesc(MCI, IVec, InstructionAddress);
if (!DescOrErr)
return DescOrErr.takeError();
const InstrDesc &D = *DescOrErr;
diff --git a/llvm/tools/llvm-mca/llvm-mca.cpp b/llvm/tools/llvm-mca/llvm-mca.cpp
index 03d7d7944b9cd..d497d67efbd9b 100644
--- a/llvm/tools/llvm-mca/llvm-mca.cpp
+++ b/llvm/tools/llvm-mca/llvm-mca.cpp
@@ -611,8 +611,8 @@ int main(int argc, char **argv) {
const SmallVector<mca::Instrument *> Instruments =
InstrumentRegions.getActiveInstruments(Loc);
- Expected<std::unique_ptr<mca::Instruction>> Inst =
- IB.createInstruction(MCI, Instruments);
+ Expected<std::unique_ptr<mca::Instruction>> Inst = IB.createInstruction(
+ MCI, Instruments, reinterpret_cast<uint64_t>(&MCI));
if (!Inst) {
if (auto NewE = handleErrors(
Inst.takeError(),
diff --git a/llvm/unittests/tools/llvm-mca/MCATestBase.cpp b/llvm/unittests/tools/llvm-mca/MCATestBase.cpp
index 4f444fae3d4c3..4032d5bdf4bd2 100644
--- a/llvm/unittests/tools/llvm-mca/MCATestBase.cpp
+++ b/llvm/unittests/tools/llvm-mca/MCATestBase.cpp
@@ -71,8 +71,8 @@ Error MCATestBase::runBaselineMCA(json::Object &Result, ArrayRef<MCInst> Insts,
const SmallVector<mca::Instrument *> Instruments;
SmallVector<std::unique_ptr<mca::Instruction>> LoweredInsts;
for (const auto &MCI : Insts) {
- Expected<std::unique_ptr<mca::Instruction>> Inst =
- IB.createInstruction(MCI, Instruments);
+ Expected<std::unique_ptr<mca::Instruction>> Inst = IB.createInstruction(
+ MCI, Instruments, reinterpret_cast<uint64_t>(&MCI));
if (!Inst) {
if (auto NewE =
handleErrors(Inst.takeError(),
diff --git a/llvm/unittests/tools/llvm-mca/X86/TestIncrementalMCA.cpp b/llvm/unittests/tools/llvm-mca/X86/TestIncrementalMCA.cpp
index 00a44dc1bab15..e3cb922462e90 100644
--- a/llvm/unittests/tools/llvm-mca/X86/TestIncrementalMCA.cpp
+++ b/llvm/unittests/tools/llvm-mca/X86/TestIncrementalMCA.cpp
@@ -40,7 +40,7 @@ TEST_F(X86TestBase, TestResumablePipeline) {
for (unsigned i = 0U, E = MCIs.size(); i < E;) {
for (unsigned TE = i + 7; i < TE && i < E; ++i) {
Expected<std::unique_ptr<mca::Instruction>> InstOrErr =
- IB.createInstruction(MCIs[i], Instruments);
+ IB.createInstruction(MCIs[i], Instruments, i);
ASSERT_TRUE(bool(InstOrErr));
ISM.addInst(std::move(InstOrErr.get()));
}
@@ -132,7 +132,7 @@ TEST_F(X86TestBase, TestInstructionRecycling) {
for (unsigned i = 0U, E = MCIs.size(); i < E;) {
for (unsigned TE = i + 7; i < TE && i < E; ++i) {
Expected<std::unique_ptr<mca::Instruction>> InstOrErr =
- IB.createInstruction(MCIs[i], Instruments);
+ IB.createInstruction(MCIs[i], Instruments, i);
if (!InstOrErr) {
mca::Instruction *RecycledInst = nullptr;
More information about the llvm-commits
mailing list