From 56674e8e4a4bb086a03857ec28739b1ecbd05374 Mon Sep 17 00:00:00 2001 From: Michael Maitland Date: Wed, 17 May 2023 13:48:18 -0700 Subject: [PATCH] [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 --- llvm/include/llvm/MCA/CustomBehaviour.h | 11 +++++------ llvm/include/llvm/MCA/InstrBuilder.h | 8 +++----- llvm/lib/MCA/CustomBehaviour.cpp | 6 +++--- llvm/lib/MCA/InstrBuilder.cpp | 6 +++--- llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.cpp | 8 ++++---- llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.h | 4 ++-- llvm/tools/llvm-mca/CodeRegion.cpp | 11 ++++++----- llvm/tools/llvm-mca/CodeRegion.h | 17 ++++++++++------- llvm/tools/llvm-mca/CodeRegionGenerator.cpp | 4 ++-- llvm/tools/llvm-mca/llvm-mca.cpp | 2 +- llvm/unittests/tools/llvm-mca/MCATestBase.cpp | 2 +- .../unittests/tools/llvm-mca/X86/TestIncrementalMCA.cpp | 4 ++-- 12 files changed, 42 insertions(+), 41 deletions(-) diff --git a/llvm/include/llvm/MCA/CustomBehaviour.h b/llvm/include/llvm/MCA/CustomBehaviour.h index e2a7ad1..348f2e8 100644 --- a/llvm/include/llvm/MCA/CustomBehaviour.h +++ b/llvm/include/llvm/MCA/CustomBehaviour.h @@ -133,7 +133,7 @@ public: StringRef getData() const { return Data; } }; -using SharedInstrument = std::shared_ptr; +using UniqueInstrument = std::unique_ptr; /// 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 @@ public: // 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 @@ public: /// 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 &IVec) const; + virtual unsigned getSchedClassID(const MCInstrInfo &MCII, const MCInst &MCI, + const SmallVector &IVec) const; }; } // namespace mca diff --git a/llvm/include/llvm/MCA/InstrBuilder.h b/llvm/include/llvm/MCA/InstrBuilder.h index cca71bb..c8619af 100644 --- a/llvm/include/llvm/MCA/InstrBuilder.h +++ b/llvm/include/llvm/MCA/InstrBuilder.h @@ -84,11 +84,10 @@ class InstrBuilder { InstRecycleCallback InstRecycleCB; Expected - createInstrDescImpl(const MCInst &MCI, - const SmallVector &IVec); + createInstrDescImpl(const MCInst &MCI, const SmallVector &IVec); Expected getOrCreateInstrDesc(const MCInst &MCI, - const SmallVector &IVec); + const SmallVector &IVec); InstrBuilder(const InstrBuilder &) = delete; InstrBuilder &operator=(const InstrBuilder &) = delete; @@ -114,8 +113,7 @@ public: void setInstRecycleCallback(InstRecycleCallback CB) { InstRecycleCB = CB; } Expected> - createInstruction(const MCInst &MCI, - const SmallVector &IVec); + createInstruction(const MCInst &MCI, const SmallVector &IVec); }; } // namespace mca } // namespace llvm diff --git a/llvm/lib/MCA/CustomBehaviour.cpp b/llvm/lib/MCA/CustomBehaviour.cpp index b593e96..a690850 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>(); } -SharedInstrument InstrumentManager::createInstrument(llvm::StringRef Desc, +UniqueInstrument InstrumentManager::createInstrument(llvm::StringRef Desc, llvm::StringRef Data) { - return std::make_shared(Desc, Data); + return std::make_unique(Desc, Data); } unsigned InstrumentManager::getSchedClassID( const MCInstrInfo &MCII, const MCInst &MCI, - const llvm::SmallVector &IVec) const { + const llvm::SmallVector &IVec) const { return MCII.get(MCI.getOpcode()).getSchedClass(); } diff --git a/llvm/lib/MCA/InstrBuilder.cpp b/llvm/lib/MCA/InstrBuilder.cpp index 7ca5513..bddd370 100644 --- a/llvm/lib/MCA/InstrBuilder.cpp +++ b/llvm/lib/MCA/InstrBuilder.cpp @@ -511,7 +511,7 @@ Error InstrBuilder::verifyInstrDesc(const InstrDesc &ID, Expected InstrBuilder::createInstrDescImpl(const MCInst &MCI, - const SmallVector &IVec) { + const SmallVector &IVec) { assert(STI.getSchedModel().hasInstrSchedModel() && "Itineraries are not yet supported!"); @@ -601,7 +601,7 @@ InstrBuilder::createInstrDescImpl(const MCInst &MCI, Expected InstrBuilder::getOrCreateInstrDesc(const MCInst &MCI, - const SmallVector &IVec) { + const SmallVector &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> InstrBuilder::createInstruction(const MCInst &MCI, - const SmallVector &IVec) { + const SmallVector &IVec) { Expected 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 ada5905..00ffde0 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(Data); + return std::make_unique(Data); } unsigned RISCVInstrumentManager::getSchedClassID( const MCInstrInfo &MCII, const MCInst &MCI, - const llvm::SmallVector &IVec) const { + const llvm::SmallVector &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(I.get())->getLMUL(); + uint8_t LMUL = static_cast(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 ce35b1b..7ce072b 100644 --- a/llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.h +++ b/llvm/lib/Target/RISCV/MCA/RISCVCustomBehaviour.h @@ -47,13 +47,13 @@ public: 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 &IVec) const override; + const SmallVector &IVec) const override; }; } // namespace mca diff --git a/llvm/tools/llvm-mca/CodeRegion.cpp b/llvm/tools/llvm-mca/CodeRegion.cpp index c91ed75..ba51880 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(Description, Loc, I)); + Regions.emplace_back( + std::make_unique(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 +const SmallVector InstrumentRegions::getActiveInstruments(SMLoc Loc) const { - SmallVector AI; + SmallVector AI; for (auto &R : Regions) { if (R->isLocInRange(Loc)) { InstrumentRegion *IR = static_cast(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 f9b999f..61ee40b 100644 --- a/llvm/tools/llvm-mca/CodeRegion.h +++ b/llvm/tools/llvm-mca/CodeRegion.h @@ -91,6 +91,8 @@ public: 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 @@ protected: public: CodeRegions(llvm::SourceMgr &S) : SM(S), FoundErrors(false) {} + virtual ~CodeRegions() = default; typedef std::vector::iterator iterator; typedef std::vector::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 - getActiveInstruments(llvm::SMLoc Loc) const; + const SmallVector getActiveInstruments(llvm::SMLoc Loc) const; }; } // namespace mca diff --git a/llvm/tools/llvm-mca/CodeRegionGenerator.cpp b/llvm/tools/llvm-mca/CodeRegionGenerator.cpp index b8e10fa..8321cfb 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 ff29e39..23b537e 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> LoweredSequence; for (const MCInst &MCI : Insts) { SMLoc Loc = MCI.getLoc(); - const SmallVector Instruments = + const SmallVector Instruments = InstrumentRegions.getActiveInstruments(Loc); Expected> Inst = diff --git a/llvm/unittests/tools/llvm-mca/MCATestBase.cpp b/llvm/unittests/tools/llvm-mca/MCATestBase.cpp index 543edbf..4f444fae 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 Insts, auto IM = std::make_unique(*STI, *MCII); mca::InstrBuilder IB(*STI, *MCII, *MRI, MCIA.get(), *IM); - const SmallVector Instruments; + const SmallVector Instruments; SmallVector> LoweredInsts; for (const auto &MCI : Insts) { Expected> Inst = diff --git a/llvm/unittests/tools/llvm-mca/X86/TestIncrementalMCA.cpp b/llvm/unittests/tools/llvm-mca/X86/TestIncrementalMCA.cpp index a0b743e..00a44dc 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(*STI, *MCII); mca::InstrBuilder IB(*STI, *MCII, *MRI, MCIA.get(), *IM); - const SmallVector Instruments; + const SmallVector 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 Instruments; + const SmallVector Instruments; // Tile size = 7 for (unsigned i = 0U, E = MCIs.size(); i < E;) { for (unsigned TE = i + 7; i < TE && i < E; ++i) { -- 2.7.4