This is an archive of the discontinued LLVM Phabricator instance.

[llvm-mca] Improved error handling and error reporting from class InstrBuilder.
ClosedPublic

Authored by andreadb on Oct 23 2018, 9:33 AM.

Details

Summary

This patch adds a new class named InstructionError to improve the error reporting in the driver.

A new class named InstructionError has been added to Support.h in order to improve the error reporting from class InstrBuilder.
The llvm-mca driver is responsible for handling InstructionError objects, and printing them out to stderr.

The goal of this patch is to remove all the remaining error handling logic from the library code.
In particular, this allows us to:

  • Simplify the logic in InstrBuilder by removing a needless dependency from MCInstrPrinter.
  • Centralize all the error halding logic in a new function named 'runPipeline' (see llvm-mca.cpp).

This is also a first step towards generalizing class InstrBuilder, so that in future, we will be able to reuse its logic to also "lower" MachineInstr to mca::Instruction objects.

Let me know if okay to commit.

Diff Detail

Repository
rL LLVM

Event Timeline

andreadb created this revision.Oct 23 2018, 9:33 AM
mattd added a comment.EditedOct 23 2018, 2:22 PM

LGTM. I have a few comments inline, but the code looks nice.

tools/llvm-mca/lib/InstrBuilder.cpp
334 ↗(On Diff #170666)

You can probably get away with just making these StringRefs in this case.

341 ↗(On Diff #170666)

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 "
tools/llvm-mca/llvm-mca.cpp
331 ↗(On Diff #170666)

This function can be marked as a static.
STI can be made const.

mattd accepted this revision.Oct 23 2018, 2:23 PM
This revision is now accepted and ready to land.Oct 23 2018, 2:23 PM

Thanks for the review Matt.

tools/llvm-mca/lib/InstrBuilder.cpp
334 ↗(On Diff #170666)

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.

341 ↗(On Diff #170666)

Will do.

tools/llvm-mca/llvm-mca.cpp
331 ↗(On Diff #170666)

Right. I will change it.

This revision was automatically updated to reflect the committed changes.