[PATCH] D50561: [llvm-mca] Propagate fatal llvm-mca errors from library classes to driver.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added inline comments.


================
Comment at: tools/llvm-mca/InstrBuilder.cpp:218-221
   if (CurrentDef != NumExplicitDefs)
-    llvm::report_fatal_error(
-        "error: Expected more register operand definitions. ");
+    return make_error<StringError>(
+        "error: Expected more register operand definitions.",
+        inconvertibleErrorCode());
----------------
Surround this statement with braces.


================
Comment at: tools/llvm-mca/InstrBuilder.cpp:287-289
+    return make_error<StringError>(
+        "error: Expected more register operand definitions. ",
+        inconvertibleErrorCode());
----------------
Same.


================
Comment at: tools/llvm-mca/InstrBuilder.cpp:343-344
     if (!SchedClassID)
-      llvm::report_fatal_error("unable to resolve this variant class.");
+      return make_error<StringError>("unable to resolve this variant class.",
+                                     inconvertibleErrorCode());
   }
----------------
Same.


================
Comment at: tools/llvm-mca/InstrBuilder.cpp:350
   if (SCDesc.NumMicroOps == MCSchedClassDesc::InvalidNumMicroOps) {
-    std::string ToString;
+    std::string ToString = "note: instruction: ";
     llvm::raw_string_ostream OS(ToString);
----------------
Revert this change. `WithColor::note()` already adds string prefix "note:".
Also, you are printing "instruction:" twice.


================
Comment at: tools/llvm-mca/Pipeline.cpp:88
+      return Val.takeError();
+    if (*Val == Stage::Stop)
       break;
----------------
I wonder if it is more readable to use `Val.get()` here... It is up to you.


================
Comment at: tools/llvm-mca/RegisterFile.h:160-162
+  // In the case that the instruction requires more than the number of
+  // registers in a register file, a warning is issued and the register file is
+  // adjusted to accommodate the register count of the instruction.
----------------
There is no warning generated from that method (there is only a debug print).
I suggest to just drop this comment.


================
Comment at: tools/llvm-mca/Stage.h:42
+  /// that is provided by the Expected template.
+  enum State { Stop, Continue }; using Status = llvm::Expected<State>;
+
----------------
Add a newline after the semicolon.


https://reviews.llvm.org/D50561