[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 ↗

(On Diff #172834)

this can be moved to the cpp file.

57 ↗

(On Diff #172834)

ditto

tools/llvm-mca/CodeRegion.h

40 ↗

(On Diff #172834)

why ?

130 ↗

(On Diff #172834)

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

tools/llvm-mca/llvm-mca.cpp

344 ↗

(On Diff #172834)

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 ↗

(On Diff #172834)

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 ↗

(On Diff #172834)

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 ↗

(On Diff #172834)

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 ↗

(On Diff #172834)

I had the same question..

tools/llvm-mca/llvm-mca.cpp

44 ↗

(On Diff #172834)

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.

llvm/trunk/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
   CodeRegion.cpp
+  CodeRegionGenerator.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)

llvm/trunk/tools/llvm-mca/CodeRegion.h

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();

llvm/trunk/tools/llvm-mca/CodeRegionGenerator.h

+//===----------------------- CodeRegionGenerator.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 classes responsible for generating llvm-mca
+/// CodeRegions from various types of input. llvm-mca only analyzes CodeRegions,
+/// so the classes here provide the input-to-CodeRegions translation.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_TOOLS_LLVM_MCA_CODEREGION_GENERATOR_H
+#define LLVM_TOOLS_LLVM_MCA_CODEREGION_GENERATOR_H
+
+#include "CodeRegion.h"
+#include "llvm/MC/MCAsmInfo.h"
+#include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCSubtargetInfo.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/SourceMgr.h"
+#include "llvm/Support/TargetRegistry.h"
+#include <memory>
+
+namespace llvm {
+namespace mca {
+
+/// 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 ~CodeRegionGenerator();
+  virtual Expected<const CodeRegions &> parseCodeRegions() = 0;
+};
+
+/// 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.
+
+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) {}
+
+  unsigned getAssemblerDialect() const { return AssemblerDialect; }
+  Expected<const CodeRegions &> parseCodeRegions() override;
+};
+
+} // namespace mca
+} // namespace llvm
+
+#endif // LLVM_TOOLS_LLVM_MCA_CODEREGION_GENERATOR_H

llvm/trunk/tools/llvm-mca/CodeRegionGenerator.cpp

+//===----------------------- CodeRegionGenerator.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 defines classes responsible for generating llvm-mca
+/// CodeRegions from various types of input. llvm-mca only analyzes CodeRegions,
+/// so the classes here provide the input-to-CodeRegions translation.
+//
+//===----------------------------------------------------------------------===//
+
+#include "CodeRegionGenerator.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/MC/MCParser/MCTargetAsmParser.h"
+#include "llvm/MC/MCStreamer.h"
+#include "llvm/MC/MCTargetOptions.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/SMLoc.h"
+#include <memory>
+
+namespace llvm {
+namespace mca {
+
+// This virtual dtor serves as the anchor for the CodeRegionGenerator class.
+CodeRegionGenerator::~CodeRegionGenerator() {}
+
+// 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;
+};
+
+// This class provides the callbacks that occur when parsing input assembly.
+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);
+  }
+};
+
+void 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

llvm/trunk/tools/llvm-mca/llvm-mca.cpp

 //
 // The target defaults to the host target.
 // The cpu defaults to the 'native' host cpu.
 // The output defaults to standard output.
 //
 //===----------------------------------------------------------------------===//
 
 #include "CodeRegion.h"
+#include "CodeRegionGenerator.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/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) {
   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 "