This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel][AArch64] Adding -disable-gisel-legality-check CL option
ClosedPublic

Authored by rtereshin on Feb 3 2018, 12:20 PM.

Details

Summary

Currently it's impossible to test InstructionSelect pass with MIR which
is considered illegal by the Legalizer in Assert builds. In early stages
of porting an existing backend from SelectionDAG ISel to GlobalISel,
however, we would have very basic CallLowering, Legalizer, and
RegBankSelect implementations, but rather functional Instruction Select
with quite a few patterns selectable due to the semi-automatic porting
process borrowing them from SelectionDAG ISel.

As we are trying to define legality as a property of being selectable by
the instruction selector, it would be nice to be able to easily check
what the selector can do in its current state w/o the legality check
provided by the Legalizer getting in the way.

It also seems beneficial to have a regression testing set up that would
not allow the selector to silently regress in its support of the MIR not
supported yet by the previous passes in the GlobalISel pipeline.

This commit adds -disable-gisel-legality-check command line option to
llc that disables those legality checks in RegBankSelect and
InstructionSelect passes.

It also adds quite a few MIR test cases for AArch64's Instruction
Selector. Every one of them would fail on the legality check at the
moment, but will select just fine if the check is disabled. Every test
MachineFunction is intended to exercise a specific selection rule and
that rule only, encoded in the MachineFunction's name by the rule's
number, ID, and index of its GIM_Try opcode in TableGen'erated
MatchTable (-optimize-match-table=false).

Diff Detail

Repository
rL LLVM

Event Timeline

rtereshin created this revision.Feb 3 2018, 12:20 PM

This seems like a bit of a hack compared to actually fixing the TODO and moving this check into the MachineVerifier where it belongs.

rtereshin added a comment.EditedFeb 5 2018, 11:10 AM

@bogner

This seems like a bit of a hack compared to actually fixing the TODO and moving this check into the MachineVerifier where it belongs.

Hi Justin,

I think this patch and fixing the TODO are orthogonal to each other: moving the check into MachineVerifier doesn't solve the problem stated as it will be still impossible to actually run-pass instruction-select on a MIR file that is considered illegal by the current implementation of the legalizer, as far as I can tell. The reason being we run MachineVerifier unconditionally upon loading a MIR file, and if it includes this check, it will fail.

So making this check explicitly optional independently from other checks MachineVerifier does and including that check into the MachineVerifier are 2 different things.

What do you think?

I'm updating the test here soon, as over time number of test cases that wouldn't be possible w/o disabling legality check probably shrinks a little (as the legalizer is getting better)

rtereshin edited the summary of this revision. (Show Details)Feb 26 2018, 5:35 PM
rtereshin added a reviewer: bogner.
rtereshin updated this revision to Diff 136017.Feb 26 2018, 5:51 PM

I've updated the test

rtereshin edited the summary of this revision. (Show Details)Feb 26 2018, 5:58 PM
rtereshin added a reviewer: rovka.
bogner accepted this revision.Feb 27 2018, 10:55 AM

Okay, I guess this makes sense for now. I think we should name the flag -disable-mir-legality-check instead of -disable-legality-check, as the current name is a bit too general sounding.

Do you need me to commit this for you?

This revision is now accepted and ready to land.Feb 27 2018, 10:55 AM

@bogner

Hi Justin,

Okay, I guess this makes sense for now.

Thanks!

I had a brief discussion with Quentin @qcolombet about this, he suggested that moving that legality check to where it belongs - closer to Machine Verifier - might be just another use case of having customizable / extendable Machine Verifier, so different parts of the generic pipeline, like in this case, as well as different targets could (temporary / locally) modify what checks Machine Verifier performs, or drive it a little. Like custom (and potentially target-specific) pre- and post- conditions on every (machine) pass, for instance. Providing API for something like this might decouple Machine Verifier from its clients just enough to solve this "but Legalizer is in a different library..." problem: let's say, the legality check is performed by Machine Verifier, but its implementation still goes with the Legalizer, while RegBankSelect and InstructionSelect passes simply register that additional check with the Machine Verifier. Something along these lines. This kind of work is probably well outside of this patch scope.

I think we should name the flag -disable-mir-legality-check instead of -disable-legality-check, as the current name is a bit too general sounding.

Sure, I'll rename it. Specifically mir-, or maybe gisel- or globalisel-?

Do you need me to commit this for you?

I'm waiting on my commit access, I think I'd wait a few days, if I have it by then, I'll commit this myself, if not, I'll let you know, thank you for offering, much appreciated!

BTW, the list of the tests that would fail the legality check, but select just fine is actually about 20-25% longer, but I can't add them as they all contain intrinsic calls and https://reviews.llvm.org/D43267 blocks them. Technically, I could add a dot or something to every intrinsic's name to "fix" it, but this is not the way MIR is printed and I'd rather wait on MIRParser fix to get in.

rtereshin updated this revision to Diff 136424.Feb 28 2018, 4:18 PM
rtereshin retitled this revision from [GlobalISel][AArch64] Adding -disable-legality-check CL option to [GlobalISel][AArch64] Adding -disable-gisel-legality-check CL option.
rtereshin edited the summary of this revision. (Show Details)

Renaming disable-legality-check to disable-gisel-legality-check as discussed (offline)

This revision was automatically updated to reflect the committed changes.
lib/CodeGen/GlobalISel/RegBankSelect.cpp