[PATCH] D153376: Introducing llvm-cm: A Cost Model Tool

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/test/tools/llvm-cm/inst_count.s:100
+
+# CHECK: # of instructions:
+
----------------
might as well test how many instructions


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:65
+static cl::opt<StringRef> CPU("mcpu", 
+                              cl::desc("Target a specific cpu type (-mcpu=help for details)"), 
+                              cl::init("skylake"), 
----------------
`-mcpu=help` works? if not (or not right now), remove that part from `cl::desc`.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:70
+// Class for error handling.  
+class ExitIf {
+   public:
----------------
nit: `class ExitIf final`

upon looking more closely at the usage pattern - @ondrasej , why raii and not just a function call? raii will exit at scope exit, which is undesirable (the goal is to exit right away). A function call "just works" because `exit(1)`, no?


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:81
+   private:
+    bool Condition;
+    std::string Message; 
----------------
`bool Condition =false` or `const bool Condition`


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:180
+// Rewrite the printFunction function to only take in aliases.
+void printFunction(ArrayRef<SymbolInfoTy> &Aliases) {
+  for (size_t I = 0; I < Aliases.size(); ++I) {
----------------
nit: `printFunctionNames` - otherwise it sounds like you're printing the whole thing?


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:188
+// TODO: Share this with llvm-objdump.cpp. 
+static void collectBBtoAddressLabels(const std::unordered_map<uint64_t, llvm::object::BBAddrMap> &AddrToBBAddrMap,
+                       uint64_t SectionAddr, uint64_t Start, uint64_t End,
----------------
this is from subsequent patch?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153376/new/

https://reviews.llvm.org/D153376