[llvm] 56674e8 - [llvm-mca][RISCV] Fix llvm-mca RISCVInstrument memory leak
Michael Maitland via llvm-commits
llvm-commits at lists.llvm.org
Mon May 22 10:36:54 PDT 2023
Author: Michael Maitland
Date: 2023-05-22T10:36:41-07:00
New Revision: 56674e8e4a4bb086a03857ec28739b1ecbd05374
URL: https://github.com/llvm/llvm-project/commit/56674e8e4a4bb086a03857ec28739b1ecbd05374
DIFF: https://github.com/llvm/llvm-project/commit/56674e8e4a4bb086a03857ec28739b1ecbd05374.diff
LOG: [llvm-mca][RISCV] Fix llvm-mca RISCVInstrument memory leak
There was a memory leak that presented itself once the llvm-mca
tests were committed. This leak was not checked for by the pre-commit
tests. This change changes the shared_ptr to a unique_ptr to avoid
this problem.
We will know that this fix works once committed since I don't know
whether it is possible to force a lit test to use LSan. I spent the
day trying to build llvm with LSan enabled without much luck. If
anyone knows how to build llvm with LSan for the lit-tests, I am
happy to give it another try locally.
Differential Revision: https://reviews.llvm.org/D150816
Added:
Modified:
llvm/include/llvm/MCA/CustomBehaviour.h
llvm/include/llvm/MCA/InstrBuilder.h
llvm/lib/MCA/CustomBehaviour.cpp
llvm/lib/MCA/InstrBuilder.cpp
llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.cpp
llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.h
llvm/tools/llvm-mca/CodeRegion.cpp
llvm/tools/llvm-mca/CodeRegion.h
llvm/tools/llvm-mca/CodeRegionGenerator.cpp
llvm/tools/llvm-mca/llvm-mca.cpp
llvm/unittests/tools/llvm-mca/MCATestBase.cpp
llvm/unittests/tools/llvm-mca/X86/TestIncrementalMCA.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/MCA/CustomBehaviour.h b/llvm/include/llvm/MCA/CustomBehaviour.h
index e2a7ad1b28704..348f2e869e20c 100644
--- a/llvm/include/llvm/MCA/CustomBehaviour.h
+++ b/llvm/include/llvm/MCA/CustomBehaviour.h
@@ -133,7 +133,7 @@ class Instrument {
StringRef getData() const { return Data; }
};
-using SharedInstrument = std::shared_ptr<Instrument>;
+using UniqueInstrument = std::unique_ptr<Instrument>;
/// This class allows targets to optionally customize the logic that resolves
/// scheduling class IDs. Targets can use information encoded in Instrument
@@ -156,8 +156,8 @@ class InstrumentManager {
// Instrument.Desc equal to Type
virtual bool supportsInstrumentType(StringRef Type) const { return false; }
- /// Allocate an Instrument, and return a shared pointer to it.
- virtual SharedInstrument createInstrument(StringRef Desc, StringRef Data);
+ /// Allocate an Instrument, and return a unique pointer to it.
+ virtual UniqueInstrument createInstrument(StringRef Desc, StringRef Data);
/// Given an MCInst and a vector of Instrument, a target can
/// return a SchedClassID. This can be used by a subtarget to return a
@@ -165,9 +165,8 @@ class InstrumentManager {
/// BaseInstruction This can be useful when a BaseInstruction does not convey
/// the correct scheduling information without additional data. By default,
/// it returns the SchedClassID that belongs to MCI.
- virtual unsigned
- getSchedClassID(const MCInstrInfo &MCII, const MCInst &MCI,
- const SmallVector<SharedInstrument> &IVec) const;
+ virtual unsigned getSchedClassID(const MCInstrInfo &MCII, const MCInst &MCI,
+ const SmallVector<Instrument *> &IVec) const;
};
} // namespace mca
diff --git a/llvm/include/llvm/MCA/InstrBuilder.h b/llvm/include/llvm/MCA/InstrBuilder.h
index cca71bbdff992..c8619af04b330 100644
--- a/llvm/include/llvm/MCA/InstrBuilder.h
+++ b/llvm/include/llvm/MCA/InstrBuilder.h
@@ -84,11 +84,10 @@ class InstrBuilder {
InstRecycleCallback InstRecycleCB;
Expected<const InstrDesc &>
- createInstrDescImpl(const MCInst &MCI,
- const SmallVector<SharedInstrument> &IVec);
+ createInstrDescImpl(const MCInst &MCI, const SmallVector<Instrument *> &IVec);
Expected<const InstrDesc &>
getOrCreateInstrDesc(const MCInst &MCI,
- const SmallVector<SharedInstrument> &IVec);
+ const SmallVector<Instrument *> &IVec);
InstrBuilder(const InstrBuilder &) = delete;
InstrBuilder &operator=(const InstrBuilder &) = delete;
@@ -114,8 +113,7 @@ class InstrBuilder {
void setInstRecycleCallback(InstRecycleCallback CB) { InstRecycleCB = CB; }
Expected<std::unique_ptr<Instruction>>
- createInstruction(const MCInst &MCI,
- const SmallVector<SharedInstrument> &IVec);
+ createInstruction(const MCInst &MCI, const SmallVector<Instrument *> &IVec);
};
} // namespace mca
} // namespace llvm
diff --git a/llvm/lib/MCA/CustomBehaviour.cpp b/llvm/lib/MCA/CustomBehaviour.cpp
index b593e96d15123..a690850bc5ba3 100644
--- a/llvm/lib/MCA/CustomBehaviour.cpp
+++ b/llvm/lib/MCA/CustomBehaviour.cpp
@@ -42,14 +42,14 @@ CustomBehaviour::getEndViews(llvm::MCInstPrinter &IP,
return std::vector<std::unique_ptr<View>>();
}
-SharedInstrument InstrumentManager::createInstrument(llvm::StringRef Desc,
+UniqueInstrument InstrumentManager::createInstrument(llvm::StringRef Desc,
llvm::StringRef Data) {
- return std::make_shared<Instrument>(Desc, Data);
+ return std::make_unique<Instrument>(Desc, Data);
}
unsigned InstrumentManager::getSchedClassID(
const MCInstrInfo &MCII, const MCInst &MCI,
- const llvm::SmallVector<SharedInstrument> &IVec) const {
+ const llvm::SmallVector<Instrument *> &IVec) const {
return MCII.get(MCI.getOpcode()).getSchedClass();
}
diff --git a/llvm/lib/MCA/InstrBuilder.cpp b/llvm/lib/MCA/InstrBuilder.cpp
index 7ca55132117de..bddd370ea4485 100644
--- a/llvm/lib/MCA/InstrBuilder.cpp
+++ b/llvm/lib/MCA/InstrBuilder.cpp
@@ -511,7 +511,7 @@ Error InstrBuilder::verifyInstrDesc(const InstrDesc &ID,
Expected<const InstrDesc &>
InstrBuilder::createInstrDescImpl(const MCInst &MCI,
- const SmallVector<SharedInstrument> &IVec) {
+ const SmallVector<Instrument *> &IVec) {
assert(STI.getSchedModel().hasInstrSchedModel() &&
"Itineraries are not yet supported!");
@@ -601,7 +601,7 @@ InstrBuilder::createInstrDescImpl(const MCInst &MCI,
Expected<const InstrDesc &>
InstrBuilder::getOrCreateInstrDesc(const MCInst &MCI,
- const SmallVector<SharedInstrument> &IVec) {
+ const SmallVector<Instrument *> &IVec) {
// Cache lookup using SchedClassID from Instrumentation
unsigned SchedClassID = IM.getSchedClassID(MCII, MCI, IVec);
@@ -622,7 +622,7 @@ STATISTIC(NumVariantInst, "Number of MCInsts that doesn't have static Desc");
Expected<std::unique_ptr<Instruction>>
InstrBuilder::createInstruction(const MCInst &MCI,
- const SmallVector<SharedInstrument> &IVec) {
+ const SmallVector<Instrument *> &IVec) {
Expected<const InstrDesc &> DescOrErr = getOrCreateInstrDesc(MCI, IVec);
if (!DescOrErr)
return DescOrErr.takeError();
diff --git a/llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.cpp b/llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.cpp
index ada5905681d2f..00ffde0925b36 100644
--- a/llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.cpp
+++ b/llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.cpp
@@ -73,7 +73,7 @@ bool RISCVInstrumentManager::supportsInstrumentType(
return Type == RISCVLMULInstrument::DESC_NAME;
}
-SharedInstrument
+UniqueInstrument
RISCVInstrumentManager::createInstrument(llvm::StringRef Desc,
llvm::StringRef Data) {
if (Desc != RISCVLMULInstrument::DESC_NAME) {
@@ -86,19 +86,19 @@ RISCVInstrumentManager::createInstrument(llvm::StringRef Desc,
<< Data << '\n');
return nullptr;
}
- return std::make_shared<RISCVLMULInstrument>(Data);
+ return std::make_unique<RISCVLMULInstrument>(Data);
}
unsigned RISCVInstrumentManager::getSchedClassID(
const MCInstrInfo &MCII, const MCInst &MCI,
- const llvm::SmallVector<SharedInstrument> &IVec) const {
+ const llvm::SmallVector<Instrument *> &IVec) const {
unsigned short Opcode = MCI.getOpcode();
unsigned SchedClassID = MCII.get(Opcode).getSchedClass();
for (const auto &I : IVec) {
// Unknown Instrument kind
if (I->getDesc() == RISCVLMULInstrument::DESC_NAME) {
- uint8_t LMUL = static_cast<RISCVLMULInstrument *>(I.get())->getLMUL();
+ uint8_t LMUL = static_cast<RISCVLMULInstrument *>(I)->getLMUL();
const RISCVVInversePseudosTable::PseudoInfo *RVV =
RISCVVInversePseudosTable::getBaseInfo(Opcode, LMUL);
// Not a RVV instr
diff --git a/llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.h b/llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.h
index ce35b1b02fe6f..7ce072b3840ce 100644
--- a/llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.h
+++ b/llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.h
@@ -47,13 +47,13 @@ class RISCVInstrumentManager : public InstrumentManager {
bool supportsInstrumentType(StringRef Type) const override;
/// Create a Instrument for RISC-V target
- SharedInstrument createInstrument(StringRef Desc, StringRef Data) override;
+ UniqueInstrument createInstrument(StringRef Desc, StringRef Data) override;
/// Using the Instrument, returns a SchedClassID to use instead of
/// the SchedClassID that belongs to the MCI or the original SchedClassID.
unsigned
getSchedClassID(const MCInstrInfo &MCII, const MCInst &MCI,
- const SmallVector<SharedInstrument> &IVec) const override;
+ const SmallVector<Instrument *> &IVec) const override;
};
} // namespace mca
diff --git a/llvm/tools/llvm-mca/CodeRegion.cpp b/llvm/tools/llvm-mca/CodeRegion.cpp
index c91ed759ee777..ba5188076c2e7 100644
--- a/llvm/tools/llvm-mca/CodeRegion.cpp
+++ b/llvm/tools/llvm-mca/CodeRegion.cpp
@@ -115,7 +115,7 @@ void AnalysisRegions::endRegion(StringRef Description, SMLoc Loc) {
InstrumentRegions::InstrumentRegions(llvm::SourceMgr &S) : CodeRegions(S) {}
void InstrumentRegions::beginRegion(StringRef Description, SMLoc Loc,
- SharedInstrument I) {
+ UniqueInstrument I) {
if (Description.empty()) {
SM.PrintMessage(Loc, llvm::SourceMgr::DK_Error,
"anonymous instrumentation regions are not permitted");
@@ -137,7 +137,8 @@ void InstrumentRegions::beginRegion(StringRef Description, SMLoc Loc,
}
ActiveRegions[Description] = Regions.size();
- Regions.emplace_back(std::make_unique<InstrumentRegion>(Description, Loc, I));
+ Regions.emplace_back(
+ std::make_unique<InstrumentRegion>(Description, Loc, std::move(I)));
}
void InstrumentRegions::endRegion(StringRef Description, SMLoc Loc) {
@@ -158,13 +159,13 @@ void InstrumentRegions::endRegion(StringRef Description, SMLoc Loc) {
}
}
-const SmallVector<SharedInstrument>
+const SmallVector<Instrument *>
InstrumentRegions::getActiveInstruments(SMLoc Loc) const {
- SmallVector<SharedInstrument> AI;
+ SmallVector<Instrument *> AI;
for (auto &R : Regions) {
if (R->isLocInRange(Loc)) {
InstrumentRegion *IR = static_cast<InstrumentRegion *>(R.get());
- AI.emplace_back(IR->getInstrument());
+ AI.push_back(IR->getInstrument());
}
}
return AI;
diff --git a/llvm/tools/llvm-mca/CodeRegion.h b/llvm/tools/llvm-mca/CodeRegion.h
index f9b999f645d3b..61ee40bacba06 100644
--- a/llvm/tools/llvm-mca/CodeRegion.h
+++ b/llvm/tools/llvm-mca/CodeRegion.h
@@ -91,6 +91,8 @@ class CodeRegion {
CodeRegion(llvm::StringRef Desc, llvm::SMLoc Start)
: Description(Desc), RangeStart(Start) {}
+ virtual ~CodeRegion() = default;
+
void addInstruction(const llvm::MCInst &Instruction) {
Instructions.emplace_back(Instruction);
}
@@ -115,14 +117,14 @@ using AnalysisRegion = CodeRegion;
/// in analysis of the region.
class InstrumentRegion : public CodeRegion {
/// Instrument for this region.
- SharedInstrument Instrument;
+ UniqueInstrument I;
public:
- InstrumentRegion(llvm::StringRef Desc, llvm::SMLoc Start, SharedInstrument I)
- : CodeRegion(Desc, Start), Instrument(I) {}
+ InstrumentRegion(llvm::StringRef Desc, llvm::SMLoc Start, UniqueInstrument I)
+ : CodeRegion(Desc, Start), I(std::move(I)) {}
public:
- SharedInstrument getInstrument() const { return Instrument; }
+ Instrument *getInstrument() const { return I.get(); }
};
class CodeRegionParseError final : public Error {};
@@ -142,6 +144,7 @@ class CodeRegions {
public:
CodeRegions(llvm::SourceMgr &S) : SM(S), FoundErrors(false) {}
+ virtual ~CodeRegions() = default;
typedef std::vector<UniqueCodeRegion>::iterator iterator;
typedef std::vector<UniqueCodeRegion>::const_iterator const_iterator;
@@ -179,14 +182,14 @@ struct AnalysisRegions : public CodeRegions {
};
struct InstrumentRegions : public CodeRegions {
+
InstrumentRegions(llvm::SourceMgr &S);
void beginRegion(llvm::StringRef Description, llvm::SMLoc Loc,
- SharedInstrument Instrument);
+ UniqueInstrument Instrument);
void endRegion(llvm::StringRef Description, llvm::SMLoc Loc);
- const SmallVector<SharedInstrument>
- getActiveInstruments(llvm::SMLoc Loc) const;
+ const SmallVector<Instrument *> getActiveInstruments(llvm::SMLoc Loc) const;
};
} // namespace mca
diff --git a/llvm/tools/llvm-mca/CodeRegionGenerator.cpp b/llvm/tools/llvm-mca/CodeRegionGenerator.cpp
index b8e10fa69c2dd..8321cfb5ad77c 100644
--- a/llvm/tools/llvm-mca/CodeRegionGenerator.cpp
+++ b/llvm/tools/llvm-mca/CodeRegionGenerator.cpp
@@ -184,7 +184,7 @@ void InstrumentRegionCommentConsumer::HandleComment(SMLoc Loc,
return;
}
- SharedInstrument I = IM.createInstrument(InstrumentKind, Data);
+ UniqueInstrument I = IM.createInstrument(InstrumentKind, Data);
if (!I) {
if (Data.empty())
SM.PrintMessage(Loc, llvm::SourceMgr::DK_Error,
@@ -202,7 +202,7 @@ void InstrumentRegionCommentConsumer::HandleComment(SMLoc Loc,
if (Regions.isRegionActive(InstrumentKind))
Regions.endRegion(InstrumentKind, Loc);
// Start new instrumentation region
- Regions.beginRegion(InstrumentKind, Loc, I);
+ Regions.beginRegion(InstrumentKind, Loc, std::move(I));
}
} // namespace mca
diff --git a/llvm/tools/llvm-mca/llvm-mca.cpp b/llvm/tools/llvm-mca/llvm-mca.cpp
index ff29e39bcdbf0..23b537e85cbd3 100644
--- a/llvm/tools/llvm-mca/llvm-mca.cpp
+++ b/llvm/tools/llvm-mca/llvm-mca.cpp
@@ -574,7 +574,7 @@ int main(int argc, char **argv) {
SmallVector<std::unique_ptr<mca::Instruction>> LoweredSequence;
for (const MCInst &MCI : Insts) {
SMLoc Loc = MCI.getLoc();
- const SmallVector<mca::SharedInstrument> Instruments =
+ const SmallVector<mca::Instrument *> Instruments =
InstrumentRegions.getActiveInstruments(Loc);
Expected<std::unique_ptr<mca::Instruction>> Inst =
diff --git a/llvm/unittests/tools/llvm-mca/MCATestBase.cpp b/llvm/unittests/tools/llvm-mca/MCATestBase.cpp
index 543edbf9cb519..4f444fae3d4c3 100644
--- a/llvm/unittests/tools/llvm-mca/MCATestBase.cpp
+++ b/llvm/unittests/tools/llvm-mca/MCATestBase.cpp
@@ -68,7 +68,7 @@ Error MCATestBase::runBaselineMCA(json::Object &Result, ArrayRef<MCInst> Insts,
auto IM = std::make_unique<mca::InstrumentManager>(*STI, *MCII);
mca::InstrBuilder IB(*STI, *MCII, *MRI, MCIA.get(), *IM);
- const SmallVector<mca::SharedInstrument> Instruments;
+ const SmallVector<mca::Instrument *> Instruments;
SmallVector<std::unique_ptr<mca::Instruction>> LoweredInsts;
for (const auto &MCI : Insts) {
Expected<std::unique_ptr<mca::Instruction>> Inst =
diff --git a/llvm/unittests/tools/llvm-mca/X86/TestIncrementalMCA.cpp b/llvm/unittests/tools/llvm-mca/X86/TestIncrementalMCA.cpp
index a0b743eb4ce1c..00a44dc1bab15 100644
--- a/llvm/unittests/tools/llvm-mca/X86/TestIncrementalMCA.cpp
+++ b/llvm/unittests/tools/llvm-mca/X86/TestIncrementalMCA.cpp
@@ -35,7 +35,7 @@ TEST_F(X86TestBase, TestResumablePipeline) {
auto IM = std::make_unique<mca::InstrumentManager>(*STI, *MCII);
mca::InstrBuilder IB(*STI, *MCII, *MRI, MCIA.get(), *IM);
- const SmallVector<mca::SharedInstrument> Instruments;
+ const SmallVector<mca::Instrument *> Instruments;
// Tile size = 7
for (unsigned i = 0U, E = MCIs.size(); i < E;) {
for (unsigned TE = i + 7; i < TE && i < E; ++i) {
@@ -127,7 +127,7 @@ TEST_F(X86TestBase, TestInstructionRecycling) {
mca::InstrBuilder IB(*STI, *MCII, *MRI, MCIA.get(), *IM);
IB.setInstRecycleCallback(GetRecycledInst);
- const SmallVector<mca::SharedInstrument> Instruments;
+ const SmallVector<mca::Instrument *> Instruments;
// Tile size = 7
for (unsigned i = 0U, E = MCIs.size(); i < E;) {
for (unsigned TE = i + 7; i < TE && i < E; ++i) {
More information about the llvm-commits
mailing list