[llvm-mca] Move the AssembleInput logic into its own class.

[mattd]

Summary

This patch introduces a CodeRegionGenerator class which is responsible for parsing some type of input and creating a 'CodeRegions' instance for use by llvm-mca. In the future, we will also have a CodeRegionGenerator subclass for converting an input object file into CodeRegions. For now, we only have the subclass for converting input assembly into CodeRegions.

This is mostly a NFC patch, as the logic remains close to the original, but now encapsulated in its own class and moved outside of llvm-mca.cpp.

[mattd]

I started thinking about another way of organizing this patch, to reduce all of the potential extra files. I can only really see two CodeRegion generators: one for ASM input, and the other for object file input... once we add the binary support. Perhaps, we just have a CodeRegionGenerator.{h,cpp} and put the generators for ASM and object files in those instead of having separate header/implementations for ASM and Object files. Just a thought.

[courbet]

I like the patch, I only have cosmetic comments.

tools/llvm-mca/AsmCodeRegionGenerator.h

49

this can be moved to the cpp file.

57

ditto

tools/llvm-mca/CodeRegion.h

40

why ?

130

this is missing a virtual dtor (that you might want to use as anchor BTW).

tools/llvm-mca/llvm-mca.cpp

344

Most tools do the initialization in the main, I'd rather keep this here rather than in AsmCodeRegionGenerator::AsmCodeRegionGenerator

[andreadb]

In D54179#1289783, @mattd wrote:

I started thinking about another way of organizing this patch, to reduce all of the potential extra files. I can only really see two CodeRegion generators: one for ASM input, and the other for object file input... once we add the binary support. Perhaps, we just have a CodeRegionGenerator.{h,cpp} and put the generators for ASM and object files in those instead of having separate header/implementations for ASM and Object files. Just a thought.

Sounds reasonable to me.

[andreadb]

tools/llvm-mca/AsmCodeRegionGenerator.cpp

11

This should be fixed. It is not declaring the assembly parse. It is declaring a "code region generator" for assemby code.

tools/llvm-mca/AsmCodeRegionGenerator.h

49

I agree. I think you can probably move most of this implementation into the .cpp file. It would minimize the number of required includes.

98

You should avoid this. I don't think it is okay to initialize assembly parsers multiple times.
Please remove this, and put it back into main.

tools/llvm-mca/CodeRegion.h

40

I had the same question..

tools/llvm-mca/llvm-mca.cpp

44

why do you need this?

[mattd]

Thanks for the reviews. I've updated the patch per the feedback given.

• Removed unnecessary header.
• Fixed the header comment to be clearer in CodeRegionGenerators.{h,cpp}.
• Created header and implementation files to describe the CodeRegionGenerators. The AsmCodeRegionGenerator lives in these now.
• Added an anchor and virtual dtor to CodeRegionGenerators.
• Moved the helper class from AsmCodeRegionGenerators into CodeRegionGenerators.cpp, and cleaned up the includes accordingly.

[andreadb]

LGTM.

tools/llvm-mca/CodeRegionGenerator.cpp

34 ↗

(On Diff #172979)

you can probably make this class final too.

tools/llvm-mca/AsmCodeRegionGenerator.h

+//===----------------------- AsmCodeRegionGenerator.h -----------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+/// \file
+///
+/// This file declares the ASM parser interface for llvm-mca.  This is
+/// responsible for converting ASM into CodeRegions that llvm-mca will use
+/// for analysis.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_TOOLS_LLVM_MCA_ASM_CODEREGION_GENERATOR_H
+#define LLVM_TOOLS_LLVM_MCA_ASM_CODEREGION_GENERATOR_H
+
+#include "CodeRegion.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/MC/MCAsmInfo.h"
+#include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCParser/MCTargetAsmParser.h"
+#include "llvm/MC/MCStreamer.h"
+#include "llvm/MC/MCSubtargetInfo.h"
+#include "llvm/MC/MCTargetOptions.h"
+#include "llvm/Support/SMLoc.h"
+#include "llvm/Support/SourceMgr.h"
+#include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
+#include <memory>
+
+namespace llvm {
+namespace mca {
+
+/// This class is responsible for parsing input ASM and generating
+/// a CodeRegions instance.
+class AsmCodeRegionGenerator final : public CodeRegionGenerator {
+  const Target &TheTarget;
+  MCContext &Ctx;
+  const MCAsmInfo &MAI;
+  const MCSubtargetInfo &STI;
+  const MCInstrInfo &MCII;
+  unsigned AssemblerDialect; // This is set during parsing.
+
+  // A comment consumer that parses strings.  The only valid tokens are strings.
+  class MCACommentConsumer : public AsmCommentConsumer {
+  public:
+    CodeRegions &Regions;
+
+    MCACommentConsumer(CodeRegions &R) : Regions(R) {}
+    void HandleComment(SMLoc Loc, StringRef CommentText) override;
+  };
+
+  class MCStreamerWrapper final : public MCStreamer {
+    CodeRegions &Regions;
+
+  public:
+    MCStreamerWrapper(MCContext &Context, mca::CodeRegions &R)
+        : MCStreamer(Context), Regions(R) {}
+
+    // We only want to intercept the emission of new instructions.
+    virtual void EmitInstruction(const MCInst &Inst,
+                                 const MCSubtargetInfo & /* unused */,
+                                 bool /* unused */) override {
+      Regions.addInstruction(Inst);
+    }
+
+    bool EmitSymbolAttribute(MCSymbol *Symbol,
+                             MCSymbolAttr Attribute) override {
+      return true;
+    }
+
+    void EmitCommonSymbol(MCSymbol *Symbol, uint64_t Size,
+                          unsigned ByteAlignment) override {}
+    void EmitZerofill(MCSection *Section, MCSymbol *Symbol = nullptr,
+                      uint64_t Size = 0, unsigned ByteAlignment = 0,
+                      SMLoc Loc = SMLoc()) override {}
+    void EmitGPRel32Value(const MCExpr *Value) override {}
+    void BeginCOFFSymbolDef(const MCSymbol *Symbol) override {}
+    void EmitCOFFSymbolStorageClass(int StorageClass) override {}
+    void EmitCOFFSymbolType(int Type) override {}
+    void EndCOFFSymbolDef() override {}
+
+    ArrayRef<MCInst> GetInstructionSequence(unsigned Index) const {
+      return Regions.getInstructionSequence(Index);
+    }
+  };
+
+public:
+  AsmCodeRegionGenerator(const Target &T, SourceMgr &SM, MCContext &C,
+                         const MCAsmInfo &A, const MCSubtargetInfo &S,
+                         const MCInstrInfo &I)
+      : CodeRegionGenerator(SM), TheTarget(T), Ctx(C), MAI(A), STI(S), MCII(I),
+        AssemblerDialect(0) {
+    InitializeAllAsmParsers();
+  }
+
+  unsigned getAssemblerDialect() const { return AssemblerDialect; }
+  Expected<const CodeRegions &> parseCodeRegions() override;
+};
+
+} // namespace mca
+} // namespace llvm
+
+#endif // LLVM_TOOLS_LLVM_MCA_ASM_CODEREGION_GENERATOR_H

tools/llvm-mca/AsmCodeRegionGenerator.cpp

+//===----------------------- AsmCodeRegionGenerator.cpp ---------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+/// \file
+///
+/// This file declares the ASM parser interface for llvm-mca.  This is
+/// responsible for converting ASM into CodeRegions that llvm-mca will use
+/// for analysis.
+//
+//===----------------------------------------------------------------------===//
+
+#include "AsmCodeRegionGenerator.h"
+#include "llvm/Support/Error.h"
+
+namespace llvm {
+namespace mca {
+
+void AsmCodeRegionGenerator::MCACommentConsumer::HandleComment(
+    SMLoc Loc, StringRef CommentText) {
+  // Skip empty comments.
+  StringRef Comment(CommentText);
+  if (Comment.empty())
+    return;
+
+  // Skip spaces and tabs.
+  unsigned Position = Comment.find_first_not_of(" \t");
+  if (Position >= Comment.size())
+    // We reached the end of the comment. Bail out.
+    return;
+
+  Comment = Comment.drop_front(Position);
+  if (Comment.consume_front("LLVM-MCA-END")) {
+    Regions.endRegion(Loc);
+    return;
+  }
+
+  // Try to parse the LLVM-MCA-BEGIN comment.
+  if (!Comment.consume_front("LLVM-MCA-BEGIN"))
+    return;
+
+  // Skip spaces and tabs.
+  Position = Comment.find_first_not_of(" \t");
+  if (Position < Comment.size())
+    Comment = Comment.drop_front(Position);
+  // Use the rest of the string as a descriptor for this code snippet.
+  Regions.beginRegion(Comment, Loc);
+}
+
+Expected<const CodeRegions &> AsmCodeRegionGenerator::parseCodeRegions() {
+  MCTargetOptions Opts;
+  Opts.PreserveAsmComments = false;
+  MCStreamerWrapper Str(Ctx, Regions);
+
+  // Create a MCAsmParser and setup the lexer to recognize llvm-mca ASM
+  // comments.
+  std::unique_ptr<MCAsmParser> Parser(
+      createMCAsmParser(Regions.getSourceMgr(), Ctx, Str, MAI));
+  MCAsmLexer &Lexer = Parser->getLexer();
+  MCACommentConsumer CC(Regions);
+  Lexer.setCommentConsumer(&CC);
+
+  // Create a target-specific parser and perform the parse.
+  std::unique_ptr<MCTargetAsmParser> TAP(
+      TheTarget.createMCAsmParser(STI, *Parser, MCII, Opts));
+  if (!TAP)
+    return make_error<StringError>(
+        "This target does not support assembly parsing.",
+        inconvertibleErrorCode());
+  Parser->setTargetParser(*TAP);
+  Parser->Run(false);
+
+  // Get the assembler dialect from the input.  llvm-mca will use this as the
+  // default dialect when printing reports.
+  AssemblerDialect = Parser->getAssemblerDialect();
+  return Regions;
+}
+
+} // namespace mca
+} // namespace llvm

tools/llvm-mca/CMakeLists.txt

 include_directories(include)
 
 set(LLVM_LINK_COMPONENTS
   AllTargetsAsmPrinters
   AllTargetsAsmParsers
   AllTargetsDescs
   AllTargetsDisassemblers
   AllTargetsInfos
   MC
   MCParser
   Support
   )
 
 add_llvm_tool(llvm-mca
   llvm-mca.cpp
+  AsmCodeRegionGenerator.cpp
   CodeRegion.cpp
   PipelinePrinter.cpp
   Views/DispatchStatistics.cpp
   Views/InstructionInfoView.cpp
   Views/RegisterFileStatistics.cpp
   Views/ResourcePressureView.cpp
   Views/RetireControlUnitStatistics.cpp
   Views/SchedulerStatistics.cpp
   Views/SummaryView.cpp
   Views/TimelineView.cpp
   Views/View.cpp
   )
 
 set(LLVM_MCA_SOURCE_DIR ${CURRENT_SOURCE_DIR})
 add_subdirectory(lib)
 target_link_libraries(llvm-mca PRIVATE LLVMMCA)

tools/llvm-mca/CodeRegion.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_LLVM_MCA_CODEREGION_H
 #define LLVM_TOOLS_LLVM_MCA_CODEREGION_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/MC/MCInst.h"
+#include "llvm/Support/CachePruning.h"
 #include "llvm/Support/SMLoc.h"
 #include "llvm/Support/SourceMgr.h"
 #include <vector>
 
 namespace llvm {
 namespace mca {
 
 /// A region of assembly code.
public:
   iterator begin() { return Regions.begin(); }
   iterator end() { return Regions.end(); }
   const_iterator begin() const { return Regions.cbegin(); }
   const_iterator end() const { return Regions.cend(); }
 
   void beginRegion(llvm::StringRef Description, llvm::SMLoc Loc);
   void endRegion(llvm::SMLoc Loc);
   void addInstruction(const llvm::MCInst &Instruction);
+  llvm::SourceMgr &getSourceMgr() const { return SM; }
 
   CodeRegions(llvm::SourceMgr &S) : SM(S) {
     // Create a default region for the input code sequence.
     addRegion("Default", llvm::SMLoc());
   }
 
   llvm::ArrayRef<llvm::MCInst> getInstructionSequence(unsigned Idx) const {
     return Regions[Idx]->getInstructions();
   }
 
   bool empty() const {
     return llvm::all_of(Regions, [](const std::unique_ptr<CodeRegion> &Region) {
       return Region->empty();
     });
   }
 };
 
+/// This class is responsible for parsing the input given to the llvm-mca
+/// driver, and converting that into a CodeRegions instance.
+class CodeRegionGenerator {
+protected:
+  CodeRegions Regions;
+  CodeRegionGenerator(const CodeRegionGenerator &) = delete;
+  CodeRegionGenerator &operator=(const CodeRegionGenerator &) = delete;
+
+public:
+  CodeRegionGenerator(SourceMgr &SM) : Regions(SM) {}
+  virtual Expected<const CodeRegions &> parseCodeRegions() = 0;
+};
+
 } // namespace mca
 } // namespace llvm
 
 #endif

tools/llvm-mca/llvm-mca.cpp

 //      -o <file>
 //
 // The target defaults to the host target.
 // The cpu defaults to the 'native' host cpu.
 // The output defaults to standard output.
 //
 //===----------------------------------------------------------------------===//
 
+#include "AsmCodeRegionGenerator.h"
 #include "CodeRegion.h"
 #include "PipelinePrinter.h"
 #include "Stages/FetchStage.h"
 #include "Stages/InstructionTables.h"
 #include "Views/DispatchStatistics.h"
 #include "Views/InstructionInfoView.h"
 #include "Views/RegisterFileStatistics.h"
 #include "Views/ResourcePressureView.h"
 #include "Views/RetireControlUnitStatistics.h"
 #include "Views/SchedulerStatistics.h"
 #include "Views/SummaryView.h"
 #include "Views/TimelineView.h"
 #include "include/Context.h"
 #include "include/Pipeline.h"
 #include "include/Support.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCObjectFileInfo.h"
-#include "llvm/MC/MCParser/MCTargetAsmParser.h"
 #include "llvm/MC/MCRegisterInfo.h"
-#include "llvm/MC/MCStreamer.h"
+#include "llvm/Support/CachePruning.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/SourceMgr.h"
if (!TheTarget) {
     errs() << ProgName << ": " << Error;
     return nullptr;
   }
 
   // Return the found target.
   return TheTarget;
 }
 
-// A comment consumer that parses strings.
-// The only valid tokens are strings.
-class MCACommentConsumer : public AsmCommentConsumer {
-public:
-  mca::CodeRegions &Regions;
-
-  MCACommentConsumer(mca::CodeRegions &R) : Regions(R) {}
-  void HandleComment(SMLoc Loc, StringRef CommentText) override {
-    // Skip empty comments.
-    StringRef Comment(CommentText);
-    if (Comment.empty())
-      return;
-
-    // Skip spaces and tabs
-    unsigned Position = Comment.find_first_not_of(" \t");
-    if (Position >= Comment.size())
-      // We reached the end of the comment. Bail out.
-      return;
-
-    Comment = Comment.drop_front(Position);
-    if (Comment.consume_front("LLVM-MCA-END")) {
-      Regions.endRegion(Loc);
-      return;
-    }
-
-    // Now try to parse string LLVM-MCA-BEGIN
-    if (!Comment.consume_front("LLVM-MCA-BEGIN"))
-      return;
-
-    // Skip spaces and tabs
-    Position = Comment.find_first_not_of(" \t");
-    if (Position < Comment.size())
-      Comment = Comment.drop_front(Position);
-    // Use the rest of the string as a descriptor for this code snippet.
-    Regions.beginRegion(Comment, Loc);
-  }
-};
-
-int AssembleInput(MCAsmParser &Parser, const Target *TheTarget,
-                  MCSubtargetInfo &STI, MCInstrInfo &MCII,
-                  MCTargetOptions &MCOptions) {
-  std::unique_ptr<MCTargetAsmParser> TAP(
-      TheTarget->createMCAsmParser(STI, Parser, MCII, MCOptions));
-
-  if (!TAP) {
-    WithColor::error() << "this target does not support assembly parsing.\n";
-    return 1;
-  }
-
-  Parser.setTargetParser(*TAP);
-  return Parser.Run(false);
-}
-
 ErrorOr<std::unique_ptr<ToolOutputFile>> getOutputStream() {
   if (OutputFilename == "")
     OutputFilename = "-";
   std::error_code EC;
   auto Out =
       llvm::make_unique<ToolOutputFile>(OutputFilename, EC, sys::fs::F_None);
   if (!EC)
     return std::move(Out);
   return EC;
 }
-
-class MCStreamerWrapper final : public MCStreamer {
-  mca::CodeRegions &Regions;
-
-public:
-  MCStreamerWrapper(MCContext &Context, mca::CodeRegions &R)
-      : MCStreamer(Context), Regions(R) {}
-
-  // We only want to intercept the emission of new instructions.
-  virtual void EmitInstruction(const MCInst &Inst,
-                               const MCSubtargetInfo & /* unused */,
-                               bool /* unused */) override {
-    Regions.addInstruction(Inst);
-  }
-
-  bool EmitSymbolAttribute(MCSymbol *Symbol, MCSymbolAttr Attribute) override {
-    return true;
-  }
-
-  void EmitCommonSymbol(MCSymbol *Symbol, uint64_t Size,
-                        unsigned ByteAlignment) override {}
-  void EmitZerofill(MCSection *Section, MCSymbol *Symbol = nullptr,
-                    uint64_t Size = 0, unsigned ByteAlignment = 0,
-                    SMLoc Loc = SMLoc()) override {}
-  void EmitGPRel32Value(const MCExpr *Value) override {}
-  void BeginCOFFSymbolDef(const MCSymbol *Symbol) override {}
-  void EmitCOFFSymbolStorageClass(int StorageClass) override {}
-  void EmitCOFFSymbolType(int Type) override {}
-  void EndCOFFSymbolDef() override {}
-
-  ArrayRef<MCInst> GetInstructionSequence(unsigned Index) const {
-    return Regions.getInstructionSequence(Index);
-  }
-};
 } // end of anonymous namespace
 
 static void processOptionImpl(cl::opt<bool> &O, const cl::opt<bool> &Default) {
   if (!O.getNumOccurrences() || O.getPosition() < Default.getPosition())
     O = Default.getValue();
 }
 
 static void processViewOptions() {

 }
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
 
   // Initialize targets and assembly parsers.
   llvm::InitializeAllTargetInfos();
   llvm::InitializeAllTargetMCs();
-  llvm::InitializeAllAsmParsers();
 
   // Enable printing of available targets when flag --version is specified.
   cl::AddExtraVersionPrinter(TargetRegistry::printRegisteredTargetsForVersion);
 
   cl::HideUnrelatedOptions({&ToolOptions, &ViewOptions});
 
   // Parse flags and initialize target options.
   cl::ParseCommandLineOptions(argc, argv,
                               "llvm machine code performance analyzer.\n");
 
-  MCTargetOptions MCOptions;
-  MCOptions.PreserveAsmComments = false;
-
   // Get the target from the triple. If a triple is not specified, then select
   // the default triple for the host. If the triple doesn't correspond to any
   // registered target, then exit with an error message.
   const char *ProgName = argv[0];
   const Target *TheTarget = getTarget(ProgName);
   if (!TheTarget)
     return 1;
 
int main(int argc, char **argv) {
   assert(MAI && "Unable to create target asm info!");
 
   MCObjectFileInfo MOFI;
   MCContext Ctx(MAI.get(), MRI.get(), &MOFI, &SrcMgr);
   MOFI.InitMCObjectFileInfo(TheTriple, /* PIC= */ false, Ctx);
 
   std::unique_ptr<buffer_ostream> BOS;
 
-  mca::CodeRegions Regions(SrcMgr);
-  MCStreamerWrapper Str(Ctx, Regions);
-
   std::unique_ptr<MCInstrInfo> MCII(TheTarget->createMCInstrInfo());
 
   std::unique_ptr<MCInstrAnalysis> MCIA(
       TheTarget->createMCInstrAnalysis(MCII.get()));
 
   if (!MCPU.compare("native"))
     MCPU = llvm::sys::getHostCPUName();
 
if (!STI->getSchedModel().hasInstrSchedModel()) {
 
     if (STI->getSchedModel().InstrItineraries)
       WithColor::note()
           << "cpu '" << MCPU << "' provides itineraries. However, "
           << "instruction itineraries are currently unsupported.\n";
     return 1;
   }
 
-  std::unique_ptr<MCAsmParser> P(createMCAsmParser(SrcMgr, Ctx, Str, *MAI));
-  MCAsmLexer &Lexer = P->getLexer();
-  MCACommentConsumer CC(Regions);
-  Lexer.setCommentConsumer(&CC);
-
-  if (AssembleInput(*P, TheTarget, *STI, *MCII, MCOptions))
+  // Parse the input and create CodeRegions that llvm-mca can analyze.
+  mca::AsmCodeRegionGenerator CRG(*TheTarget, SrcMgr, Ctx, *MAI, *STI, *MCII);
+  Expected<const mca::CodeRegions &> RegionsOrErr = CRG.parseCodeRegions();
+  if (auto Err = RegionsOrErr.takeError()) {
+    WithColor::error() << Err << "\n";
     return 1;
-
+  }
+  const mca::CodeRegions &Regions = *RegionsOrErr;
   if (Regions.empty()) {
     WithColor::error() << "no assembly instructions found.\n";
     return 1;
   }
 
   // Now initialize the output file.
   auto OF = getOutputStream();
   if (std::error_code EC = OF.getError()) {
     WithColor::error() << EC.message() << '\n';
     return 1;
   }
 
-  unsigned AssemblerDialect = P->getAssemblerDialect();
+  unsigned AssemblerDialect = CRG.getAssemblerDialect();
   if (OutputAsmVariant >= 0)
     AssemblerDialect = static_cast<unsigned>(OutputAsmVariant);
   std::unique_ptr<MCInstPrinter> IP(TheTarget->createMCInstPrinter(
       Triple(TripleName), AssemblerDialect, *MAI, *MCII, *MRI));
   if (!IP) {
     WithColor::error()
         << "unable to create instruction printer for target triple '"
         << TheTriple.normalize() << "' with assembly variant "