[llvm] [MCA] Parameterize variant scheduling classes by explicit variable (PR #92849)
Aiden Grossman via llvm-commits
llvm-commits at lists.llvm.org
Thu May 30 02:59:18 PDT 2024
https://github.com/boomanaiden154 updated https://github.com/llvm/llvm-project/pull/92849
>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 1/4] [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;
>From 51a8a907fe1a809d808be67c0adf64624ff65b1c Mon Sep 17 00:00:00 2001
From: Aiden Grossman <aidengrossman at google.com>
Date: Tue, 21 May 2024 03:48:34 +0000
Subject: [PATCH 2/4] Fix formatting
---
llvm/include/llvm/MCA/InstrBuilder.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/llvm/include/llvm/MCA/InstrBuilder.h b/llvm/include/llvm/MCA/InstrBuilder.h
index 67f9c736054e1..b625dfda76780 100644
--- a/llvm/include/llvm/MCA/InstrBuilder.h
+++ b/llvm/include/llvm/MCA/InstrBuilder.h
@@ -72,8 +72,7 @@ class InstrBuilder {
Descriptors;
// Key is the MCIInst and SchedClassID the describe the value InstrDesc
- DenseMap<std::pair<uint64_t, unsigned>,
- std::unique_ptr<const InstrDesc>>
+ DenseMap<std::pair<uint64_t, unsigned>, std::unique_ptr<const InstrDesc>>
VariantDescriptors;
bool FirstCallInst;
>From a0e3c4a22393fad74a0e3df522eb76baa279f5bf Mon Sep 17 00:00:00 2001
From: Aiden Grossman <aidengrossman at google.com>
Date: Thu, 30 May 2024 09:56:53 +0000
Subject: [PATCH 3/4] Add unittest
---
.../tools/llvm-mca/X86/TestIncrementalMCA.cpp | 51 +++++++++++++++++++
1 file changed, 51 insertions(+)
diff --git a/llvm/unittests/tools/llvm-mca/X86/TestIncrementalMCA.cpp b/llvm/unittests/tools/llvm-mca/X86/TestIncrementalMCA.cpp
index e3cb922462e90..48ee19f211c21 100644
--- a/llvm/unittests/tools/llvm-mca/X86/TestIncrementalMCA.cpp
+++ b/llvm/unittests/tools/llvm-mca/X86/TestIncrementalMCA.cpp
@@ -1,6 +1,8 @@
+#include "MCTargetDesc/X86MCTargetDesc.h"
#include "Views/SummaryView.h"
#include "X86TestBase.h"
#include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/MC/MCInstBuilder.h"
#include "llvm/MCA/CustomBehaviour.h"
#include "llvm/MCA/IncrementalSourceMgr.h"
#include "llvm/MCA/InstrBuilder.h"
@@ -8,6 +10,7 @@
#include "llvm/Support/Format.h"
#include "llvm/Support/JSON.h"
#include "llvm/Support/raw_ostream.h"
+#include <memory>
#include <unordered_map>
using namespace llvm;
@@ -185,3 +188,51 @@ TEST_F(X86TestBase, TestInstructionRecycling) {
ASSERT_EQ(*BV, *V) << "Value of '" << F << "' does not match";
}
}
+
+// Test that we do not depend upon the MCInst address for variant description
+// construction. This test creates two instructions that will use variant
+// description as they are both zeroing idioms, but write to different
+// registers. If the key used to access the variant instruction description is
+// the same between the descriptions (like the MCInst pointer), we will run into
+// an assertion failure due to the different writes. Within this test, setting
+// both instruction addresses to the same builder within the calls to the MCA
+// instruction builder will cause the problem.
+TEST_F(X86TestBase, TestVariantInstructionsSameAddress) {
+ mca::Context MCA(*MRI, *STI);
+
+ mca::IncrementalSourceMgr ISM;
+ // Empty CustomBehavioour.
+ auto CB = std::make_unique<mca::CustomBehaviour>(*STI, ISM, *MCII);
+
+ auto PO = getDefaultPipelineOptions();
+ auto P = MCA.createDefaultPipeline(PO, ISM, *CB);
+ ASSERT_TRUE(P);
+
+ auto IM = std::make_unique<mca::InstrumentManager>(*STI, *MCII);
+ mca::InstrBuilder IB(*STI, *MCII, *MRI, MCIA.get(), *IM);
+
+ const SmallVector<mca::Instrument *> Instruments;
+
+ MCInst InstructionToAdd;
+ InstructionToAdd = MCInstBuilder(X86::XOR64rr)
+ .addReg(X86::RAX)
+ .addReg(X86::RAX)
+ .addReg(X86::RAX);
+ Expected<std::unique_ptr<mca::Instruction>> Instruction1OrErr =
+ IB.createInstruction(InstructionToAdd, Instruments, 0);
+ ASSERT_TRUE(static_cast<bool>(Instruction1OrErr));
+ ISM.addInst(std::move(Instruction1OrErr.get()));
+
+ InstructionToAdd = MCInstBuilder(X86::XORPSrr)
+ .addReg(X86::XMM0)
+ .addReg(X86::XMM0)
+ .addReg(X86::XMM0);
+ Expected<std::unique_ptr<mca::Instruction>> Instruction2OrErr =
+ IB.createInstruction(InstructionToAdd, Instruments, 1);
+ ASSERT_TRUE(static_cast<bool>(Instruction2OrErr));
+ ISM.addInst(std::move(Instruction2OrErr.get()));
+
+ ISM.endOfStream();
+ Expected<unsigned> Cycles = P->run();
+ ASSERT_TRUE(static_cast<bool>(Cycles));
+}
>From db8a40cbd4f8cb7f4f25afb0314ad917bc08c690 Mon Sep 17 00:00:00 2001
From: Aiden Grossman <aidengrossman at google.com>
Date: Thu, 30 May 2024 09:58:28 +0000
Subject: [PATCH 4/4] Address reviewer feedback
---
llvm/include/llvm/MCA/InstrBuilder.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/llvm/include/llvm/MCA/InstrBuilder.h b/llvm/include/llvm/MCA/InstrBuilder.h
index b625dfda76780..6f227431ac2df 100644
--- a/llvm/include/llvm/MCA/InstrBuilder.h
+++ b/llvm/include/llvm/MCA/InstrBuilder.h
@@ -71,7 +71,8 @@ class InstrBuilder {
std::unique_ptr<const InstrDesc>>
Descriptors;
- // Key is the MCIInst and SchedClassID the describe the value InstrDesc
+ // Key is the instruction address and SchedClassID the describe the value
+ // InstrDesc
DenseMap<std::pair<uint64_t, unsigned>, std::unique_ptr<const InstrDesc>>
VariantDescriptors;
More information about the llvm-commits
mailing list