This is an archive of the discontinued LLVM Phabricator instance.

[llvm-mca] Propagate fatal llvm-mca errors from library classes to driver.
ClosedPublic

Authored by mattd on Aug 10 2018, 8:23 AM.

Details

Summary

This patch introduces error handling to propagate the errors from llvm-mca library classes (or what will become library classes) up to the driver. This patch also introduces an enum to make clearer the intention of the return value for Stage::execute.

This supports PR38101.

Diff Detail

Repository
rL LLVM

Event Timeline

mattd created this revision.Aug 10 2018, 8:23 AM
mattd updated this revision to Diff 160181.Aug 10 2018, 2:42 PM

Updated:

  • RegisterFile::isAvailable no longer fataly fails, instead if an instruction requires more registers than what a register file can hold, a warning is issued and the PRF is increased to hold the number of registers specified by the instruction.
  • Unfortunately this requires isAvailable to no longer be const, and that the RegisterMappingTracker to no longer have a const NumPhysRegs field.
  • Another option is check the register file lengths per instruction, but outside of isAvailable. As part of some sanity check. However, that will some what duplicate the for-loop in isAvailable.
  • I'm going to go through the patch and see if there is any template code I can simplify via using directives, I don't want to increase complexity.
mattd added inline comments.Aug 10 2018, 8:51 PM
tools/llvm-mca/RegisterFile.h
163 ↗(On Diff #160181)

I'd like to rename this to getPRFAvailabilityMask or getRegisterFileAvailability. To me, 'isAvailable' implies a boolean, which it is not in this case.

mattd updated this revision to Diff 160225.Aug 10 2018, 8:54 PM

Added Stage::Status type alias to clean up the return value for Stage::execute. The previous iteration of this patch used an Expected<bool>, which I find confusing. This change makes the code more readable.

Added Stage::Status type alias to clean up the return value for Stage::execute. The previous iteration of this patch used an Expected<bool>, which I find confusing. This change makes the code more readable.

Nice cleanup, thanks!

The code comments related to the Stage::Status should be improved.
In a follow-up patch, you should also improve the code comments in Stage.h and better describe the API.
I found our code comments in Stage.h extremely hard to read. In the end, I suspect that people have to resort at looking at the implementation to fully understand the meaning of "execute(IR)" for a stage.

I think we should do a better job at describing that API.

I have added some comments below.
If you integrate those changes, and share the patch upstream, then I will be happy to accept it.

P.s.: you should reference the llvm bugzilla related to this change, and update it accordingly.

tools/llvm-mca/InstrBuilder.cpp
350–356 ↗(On Diff #160225)

Not sure why you removed the original code that printed the note.

You should add it back:

WithColor::note() << "instruction: " << ToString << '\n';

Then you should return:

return make_error<StringError>("Don't know how to analyze unsupported instructions.", inconvertibleErrorCode());
tools/llvm-mca/RegisterFile.cpp
280 ↗(On Diff #160225)

Add const back. (see below)

305–314 ↗(On Diff #160225)

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.

tools/llvm-mca/RegisterFile.h
46 ↗(On Diff #160225)

I have been thinking about this. I think it should be reverted to const.
See my comment below for an explanation.

163 ↗(On Diff #160181)

I think it is fine to return an unsigned. If the mask is zero, then the register files are not available; any other value would be equivalent to "true" (i.e. there is at least one register file available).
So, an implicit conversion would still do the job.

tools/llvm-mca/Stage.h
34 ↗(On Diff #160225)

I think this comment (as well as other comments in the Stage.h) are misleading.
Here we are talking about the execution of a single instruction (described by an InstRef).

That statement should say that a stage returns Continue if it has successfully processed the instruction in input (identified by an InstRef), and that instruction is ready to be moved to the next stage during this same cycle.

In retrospect, most of the comments in Stage.h should be changed/improved.
I tried to read those comments the other day, and it was unclear what "execution" meant in that context. I think we should be more precise and emphasize that we are talking about execution "from an instruction point of view". Or something like that (sorry for my English). Basically, I am a bit concerned about the quality of some code comments...

tools/llvm-mca/llvm-mca.cpp
509 ↗(On Diff #160225)

You don't need to return. report_fatal_error calls exit() inside. It also returns 1 (not -1).
Please remove this return.

550 ↗(On Diff #160225)

You don't need to return. report_fatal_error calls exit() inside. It also returns 1 (not -1).
Please remove this return.

mattd updated this revision to Diff 160379.Aug 13 2018, 10:03 AM
mattd marked 5 inline comments as done.
mattd retitled this revision from [llvm-mca][wip] Idea for propagating errors to driver. to [llvm-mca] Propagate fatal llvm-mca errors from library classes to driver..
mattd edited the summary of this revision. (Show Details)
mattd added reviewers: courbet, RKSimon.
mattd added a subscriber: llvm-commits.

Addressed comments.

mattd marked 2 inline comments as done.Aug 13 2018, 10:05 AM
mattd added inline comments.
tools/llvm-mca/RegisterFile.cpp
305–314 ↗(On Diff #160225)

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.

tools/llvm-mca/llvm-mca.cpp
509 ↗(On Diff #160225)

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.

andreadb added inline comments.Aug 13 2018, 10:19 AM
tools/llvm-mca/InstrBuilder.cpp
218–221 ↗(On Diff #160379)

Surround this statement with braces.

287–289 ↗(On Diff #160379)

Same.

343–344 ↗(On Diff #160379)

Same.

350 ↗(On Diff #160379)

Revert this change. WithColor::note() already adds string prefix "note:".
Also, you are printing "instruction:" twice.

tools/llvm-mca/Pipeline.cpp
88 ↗(On Diff #160379)

I wonder if it is more readable to use Val.get() here... It is up to you.

tools/llvm-mca/RegisterFile.h
160–162 ↗(On Diff #160379)

There is no warning generated from that method (there is only a debug print).
I suggest to just drop this comment.

tools/llvm-mca/Stage.h
42 ↗(On Diff #160379)

Add a newline after the semicolon.

mattd updated this revision to Diff 160394.Aug 13 2018, 10:45 AM
mattd marked 5 inline comments as done.
  • Addressed comments:
    • Added braces.
    • Removed a stale comment and initializer.
    • Using Expected::get in a few places instead of operator* in Pipeline.cpp.
andreadb accepted this revision.Aug 13 2018, 10:51 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Aug 13 2018, 10:51 AM
This revision was automatically updated to reflect the committed changes.