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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

The pre-merge tests are still failing. Please take a look and fix the issue accordingly.



================
Comment at: llvm/test/tools/llvm-cm/empty.s:8
+
+# CHECK-NOT: {*}
----------------
This will check that the literal string "{*}" does not appear in the output. I'm guessing that's not what you meant? If you want to check that the output is empty, you should replace your FileCheck call with `count 0`.


================
Comment at: llvm/test/tools/llvm-cm/malformed.s:5
+
+bad file 
+
----------------
There's no need for this text, so I'd just delete it.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:117
+    WithColor::warning(errs(), "llvm-cm")
+        << "Failed to get section name: " << toString(SecNameOrErr.takeError())
+        << "\n";
----------------
https://llvm.org/docs/CodingStandards.html#error-and-warning-messages

Also, test case?


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:155
+  outs().flush();
+  exit(1);
+}
----------------
`std::exit`?


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:284
+  Expected<SubtargetFeatures> FeatureVals = Obj->getFeatures();
+  ExitIf(!FeatureVals) << "error: no features detected\n";
+
----------------
I don't think you want `error: ` in the message here?

Also, test case?

Same throughout.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:325
+  SectionSymbolsTy UndefinedSymbols;
+
+  bool Is64Bits = Obj->getBytesInAddress() > 4;
----------------
I don't think you want this blank line - the declarations are related to the loop, so they should be closely linked.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:328
+
+  // Get the symbol table
+  for (const object::SymbolRef &Symbol : Obj->symbols()) {
----------------
See my earlier comment about unhelpful comments. Same throughout.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:415
+
+      std::unordered_map<uint64_t, std::vector<StringRef>> BBtoAddresses;
+
----------------
Move this to where it is used. Same with a number of the other variables.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:75
+class ExitIf {
+   public:
+    ExitIf(bool Cond) : Condition(Cond) {}
----------------
jhenderson wrote:
> Have you run clang-format on your new code? Because this doesn't look formatted according to the standard format.
Ping - not addressed.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:70
+// Class for error handling.  
+class ExitIf {
+   public:
----------------
ondrasej wrote:
> mtrofin wrote:
> > 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?
> As discussed offline yesterday: RAII is there to make the streaming operators after `ExitIf()` work.
> 
> With RAII, and with the intended use (`ExitIf(Cond) << "Some message";`), the following happens:
> 1. a temporary `ExitIf` instance is created and takes note of `Cond`,
> 2. all streaming operators are applied to the temporary (collecting the messages),
> 3. it's a temporary value (there's no variable name), so its scope is limited to the statement where it appears.
> 
> Basically, it processes the chain of `<<`, and then immediately exits.
Why do you need RAII for this? You could just do string concatenation to get your final message, and pass that into a function that prints the message and then calls `std::exit`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153376