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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added inline comments.


================
Comment at: tools/llvm-mca/AsmCodeRegionGenerator.cpp:11
+///
+/// This file declares the ASM parser interface for llvm-mca.  This is
+/// responsible for converting ASM into CodeRegions that llvm-mca will use
----------------
This should be fixed. It is not declaring the assembly parse. It is declaring a "code region generator" for assemby code.


================
Comment at: tools/llvm-mca/AsmCodeRegionGenerator.h:49
+  // A comment consumer that parses strings.  The only valid tokens are strings.
+  class MCACommentConsumer : public AsmCommentConsumer {
+  public:
----------------
courbet wrote:
> this can be moved to the cpp file.
I agree. I think you can probably move most of this implementation into the .cpp file. It would minimize the number of required includes.


================
Comment at: tools/llvm-mca/AsmCodeRegionGenerator.h:98
+        AssemblerDialect(0) {
+    InitializeAllAsmParsers();
+  }
----------------
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.


================
Comment at: tools/llvm-mca/CodeRegion.h:40
 #include "llvm/MC/MCInst.h"
+#include "llvm/Support/CachePruning.h"
 #include "llvm/Support/SMLoc.h"
----------------
courbet wrote:
> why ?
I had the same question..


================
Comment at: tools/llvm-mca/llvm-mca.cpp:44
 #include "llvm/MC/MCRegisterInfo.h"
-#include "llvm/MC/MCStreamer.h"
+#include "llvm/Support/CachePruning.h"
 #include "llvm/Support/CommandLine.h"
----------------
why do you need this?


https://reviews.llvm.org/D54179