[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

diff --git a/llvm/include/llvm/MCA/CustomBehaviour.h b/llvm/include/llvm/MCA/CustomBehaviour.h
index e2a7ad1..348f2e8 100644 (file)
--- 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<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 @@ 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<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 cca71bb..c8619af 100644 (file)
--- 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 @@ public:
   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 b593e96..a690850 100644 (file)
--- 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 7ca5513..bddd370 100644 (file)
--- 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 ada5905..00ffde0 100644 (file)
--- 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 ce35b1b..7ce072b 100644 (file)
--- 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<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 c91ed75..ba51880 100644 (file)
--- 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 f9b999f..61ee40b 100644 (file)
--- 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<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 b8e10fa..8321cfb 100644 (file)
--- 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 (file)
--- 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 543edbf..4f444fa 100644 (file)
--- 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 a0b743e..00a44dc 100644 (file)
--- 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) {