[PATCH] D53585: [llvm-mca] Improved error handling and error reporting from class InstrBuilder.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Thanks for the review Matt.



================
Comment at: tools/llvm-mca/lib/InstrBuilder.cpp:334
 
-  std::string ToString;
-  raw_string_ostream OS(ToString);
+  std::string Message;
   if (UsesMemory) {
----------------
mattd wrote:
> You can probably get away with just making these StringRefs in this case.
We cannot guarantee that the storage for that string would be accessible by the time the Error object is processed. So we need to allocate a new string. Simon suggested offline to use a std::move when initializing Message. I will do that for now.


================
Comment at: tools/llvm-mca/lib/InstrBuilder.cpp:341
+    Message = "found an inconsistent instruction that decodes"
+              " to zero opcodes and that consumes scheduler "
+              "resources.";
----------------
mattd wrote:
> I know the previous message contents are the same, but for consistency, could you add the space on the preceeding line, instead of continuing the message with a leading space.
> 
> ```"found an inconsistent instruction that decodes "
> "to zero opcodes and that consumes scheduler "
> ```
Will do.


================
Comment at: tools/llvm-mca/llvm-mca.cpp:331
+// Returns true on success.
+bool runPipeline(mca::Pipeline &P, MCInstPrinter &MCIP, MCSubtargetInfo &STI) {
+  // Handle pipeline errors here.
----------------
mattd wrote:
> This function can be marked as a static.
> STI can be made const.
Right. I will change it.


https://reviews.llvm.org/D53585