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

[Matt Davis via Phabricator via llvm-commits]

mattd marked 2 inline comments as done.
mattd added inline comments.


================
Comment at: tools/llvm-mca/RegisterFile.cpp:305-314
     if (RMT.NumPhysRegs < NumRegs) {
       // The current register file is too small. This may occur if the number of
       // microarchitectural registers in register file #0 was changed by the
       // users via flag -reg-file-size. Alternatively, the scheduling model
       // specified a too small number of registers for this register file.
-      report_fatal_error(
-          "Not enough microarchitectural registers in the register file");
+      WithColor::warning()
+          << "Not enough microarchitectural registers in register file " << I
----------------
andreadb wrote:
> Here is the idea:
> 
> Normalize `NumRegs` instead:
> If `NumRegs` is bigger than `RMT.NumPhysRegs`, then silently make `NumRegs` equal to ` RMT.NumPhysRegs`.
> 
> Motivation:
> In reality, expression `RMT.NumPhysRegs < NumRegs` is never expected to be true.
> If that check fails, then there is something fundamentally wrong with the scheduling model, or the option passed in input by the user.
> In future, we may even decide to drop the driver option to control the size of a register file entirely.
> 
> Changing the API to account for a very rare case where things might go wrong is not worth it in my opinion.
> 
> I would keep this method "const", I would also add a FIXME comment explaining that the current normalization may hide an inconsistency, but because this is a rare (low priority) situation, we decided that it was not worth it to have it fixed by changing the API. This might be revisited in future.
I understand we don't want to spam the user with warnings; however, I think we should be aware of this (rare) pathological case if it occurs.  I've enabled a DEBUG message now, so that if this issue occurs, then someone investigating this in debug will be aware of the situation.


================
Comment at: tools/llvm-mca/llvm-mca.cpp:509
+        report_fatal_error(toString(std::move(Err)));
+        return -1;
+      }
----------------
andreadb wrote:
> You don't need to return. report_fatal_error calls exit() inside. It also returns 1 (not -1).
> Please remove this return.
The return is definitely redundant. My justification for adding that statement was to make it clear that the program/function ends there, a return catches the syntax highlight.


https://reviews.llvm.org/D50561