[PATCH] D43951: [RFC] llvm-mca: a static performance analysis tool.

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

This is great! I always wanted to see a IACTA like tool for other architectures.

I only glanced over the patch, so this is only the nipicky/obvious things:

- New llvm code should use `camelCase` rather than `CamelCase` for method names.
- Too much use of `auto` for my taste. I would only use it in situations where they type is immediately obvious (on the same line) such as `auto x = dyn_cast<YYY>()` but nowhere else. I find it makes code harder to read when I first have to search around what things a `for` loop is iterating over for example.
- Needs documentation in `docs/CommandGuide`. Maybe you can incorporate some passages from your llvm-dev mail.

I would love to see this tool land in tree. And as far as I am concerned we can push it and continue development in-tree when 1) waited some more for other people to chime in here, 2) we verified the normal code isn't changed too much (double check code size of MC Unit name change) 3) we have some documentation.



================
Comment at: include/llvm/MC/MCSchedule.h:27-29
-#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
   const char *Name;
-#endif
----------------
Do we have an idea how much this increases code size of other llvm tools? If this turns out to be a lot then we may be better of keeping the names either in a separate library or a separate global variables so that the linker can strip them away if unused.


================
Comment at: tools/llvm-mca/Backend.h:78
+
+  int Run() {
+    while (SM->HasNext() || !DU->IsRCUEmpty())
----------------
return value is always 0 and never used.


================
Comment at: tools/llvm-mca/Dispatch.cpp:27
+
+// Creates a new mapping for RegID.
+void RegisterFile::AddRegisterMapping(WriteState *WS) {
----------------
Move comment to the declaration.


================
Comment at: tools/llvm-mca/Dispatch.h:84
+  // Creates a new mapping for RegID.
+  void AddRegisterMapping(WriteState *WS);
+  void InvalidateRegisterMapping(const WriteState &WS);
----------------
Make WS a reference to indicate it mustn't be nullptr?


================
Comment at: tools/llvm-mca/Dispatch.h:88
+  bool IsAvailable(unsigned NumRegWrites);
+  void CollectWrites(SmallVector<WriteState *, 4> &Writes,
+                     unsigned RegID) const;
----------------
You should use `SmallVectorImpl<> &` in API to be independent of the small-size.


================
Comment at: tools/llvm-mca/Dispatch.h:304
+  void OnInstructionExecuted(unsigned TokenID) {
+    return RCU->OnInstructionExecuted(TokenID);
+  }
----------------
feels odd to have a `return` with a void value.


================
Comment at: tools/llvm-mca/InstrBuilder.cpp:113-118
+  DEBUG(for (const auto &R
+             : ID.Resources) dbgs()
+            << "\t\tMask=" << R.first << ", cy=" << R.second.size() << '\n';
+        for (const auto &R
+             : ID.Buffers) dbgs()
+        << "\t\tBuffer Mask=" << R << '\n';);
----------------
Did clang-format mess this part up?


https://reviews.llvm.org/D43951