This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][GlobalISel] Enable GlobalISel at -O0 by default
ClosedPublic

Authored by aemerson on Dec 18 2017, 10:59 AM.

Details

Summary

Tests updated to explicitly use fast-isel at -O0 instead of implicitly.

We should properly fall back to selection-dag in the cases where we don't support some feature.

This change also allows an explicit -fast-isel option to override an implicitly enabled global-isel. Otherwise -fast-isel would have no effect at -O0.

Diff Detail

Repository
rL LLVM

Event Timeline

aemerson created this revision.Dec 18 2017, 10:59 AM
qcolombet accepted this revision.Dec 18 2017, 2:59 PM

Hi Amara,

LGTM, though I would leave some time to others to reply to the review too.
I am clearly biased here :)

Cheers,
-Quentin

This revision is now accepted and ready to land.Dec 18 2017, 2:59 PM

Are you sure this is the right patch? If I try passing "-mllvm -aarch64-enable-global-isel-at-O=0" to clang on trunk, I get errors like "fatal error: error in backend: unable to lower arguments".

Sounds like this option doesn’t use the fallback path.
I thought it did.
Something to double check.

Ran some tests internally with "-mllvm -aarch64-enable-global-isel-at-O=0 -mllvm -global-isel-abort=0". Just filed https://bugs.llvm.org/show_bug.cgi?id=35690 . Otherwise seems to work.

aemerson updated this revision to Diff 127775.Dec 20 2017, 12:36 PM
aemerson edited the summary of this revision. (Show Details)
aemerson requested review of this revision.Dec 20 2017, 12:39 PM

I've updated the patch and introduced a new default behavior for -global-isel-abort so it can co-operate with targets which have GISel enabled by default. The bug Eli raised should also be fixed as well in another commit.

Sounds like this option doesn’t use the fallback path.
I thought it did.
Something to double check.

lib/CodeGen/TargetPassConfig.cpp
143

I would rather have another target hook for the abort case.
Same thing as what we have for GISel enabled (option takes precedence if set, otherwise we look at what the target wants)

Sounds like this option doesn’t use the fallback path.
I thought it did.
Something to double check.

I'm not exactly sure what you mean by this.

How about checking for EnableGlobalISelAbort.getNumOccurences() > 0 to check for an command line override, otherwise doing what the target wants via the isGlobalISelAbortEnabled() override?

How about checking for EnableGlobalISelAbort.getNumOccurences() > 0 to check for an command line override, otherwise doing what the target wants via the isGlobalISelAbortEnabled() override?

That's what I meant :). (Instead of adding a == 3 case for the option.)

Now, regarding the we should double check thing, I meant that we should double check that the -aarch64-enable-global-isel-at-O=0 properly set abort to false in the aarch64 override.

How about checking for EnableGlobalISelAbort.getNumOccurences() > 0 to check for an command line override, otherwise doing what the target wants via the isGlobalISelAbortEnabled() override?

That's what I meant :). (Instead of adding a == 3 case for the option.)

I'd be surprised if an API user want to force aborts to be enabled so it makes sense to me to check the number of occurrences of the command line option.

One possible spelling nit: If we want a target hook to control this, shouldn't the hook describe a property of the target rather than a property of the GlobalISel implementation? I'm thinking along the lines of CompleteModel in the scheduler to get something like isGlobalISelComplete() or useGlobalISelOnly()

aemerson updated this revision to Diff 127892.Dec 21 2017, 8:38 AM
aemerson edited the summary of this revision. (Show Details)

Patch updated. I've changed the abort method to no longer be an override. It doesn't really make sense to override it since only the TPC implementation can access the CLI options, so overriding it wouldn't work anyway.

We already have enough information from the isGlobalISelEnabled() hook to determine whether we should abort.

For aarch64-enable-global-isel-at-O the isGlobalISelEnabled() hook will check that value and so the abort will be implicitly disabled via that hook with this patch.

Gerolf added a subscriber: Gerolf.Jan 2 2018, 1:41 AM

Hi,

I see all issues that came up in this thread covered by the last patch. Before this can be committed I still want to check that all paths/issues are tested/covered wrt to fast-isel:
a) how do we guarantee that there is no fallback from GISel to FastISel (when GISel is supported)? This is probably a nit since it is obvious to everyone deeper in the implementation then I am.
b) for all the tests where fast-isel was added shouldn't there equivalent tests for GISel, too? Even if the tests target fast-isel specific issues how do we make sure GISel does not expose similar/same bugs?

Thanks
Gerolf

test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll
1 ↗(On Diff #127892)

Nit: -global-isel is no longer needed.

Hi,

I see all issues that came up in this thread covered by the last patch. Before this can be committed I still want to check that all paths/issues are tested/covered wrt to fast-isel:
a) how do we guarantee that there is no fallback from GISel to FastISel (when GISel is supported)? This is probably a nit since it is obvious to everyone deeper in the implementation then I am.
b) for all the tests where fast-isel was added shouldn't there equivalent tests for GISel, too? Even if the tests target fast-isel specific issues how do we make sure GISel does not expose similar/same bugs?

Thanks
Gerolf

Reading the code again I think we're covered for the fall back issue for the cases where TargetOptions::EnableFastISel is queried.

As for the tests, this change should maintain the existing test coverage of the code. You're correct in that there are cases where we're not testing the equivalent feature with global-isel, this should be follow on work after this to make GISel feature complete (rdar://35603467). I don't think adding -global-isel to the existing tests would help since we'd end up having to XFAIL them anyway. For the tests which are fast-isel specific they're mostly implementing support for something rather than bugs, and we should have tests for those as we add support over time in the GlobalISel specific tests.

test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll
1 ↗(On Diff #127892)

Ack.

Gerolf added a comment.Jan 2 2018, 7:03 AM

Thanks! This LGTM then.

-Gerolf

aemerson accepted this revision.Jan 2 2018, 8:30 AM
aemerson added inline comments.
test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll
1 ↗(On Diff #127892)

Actually is still needed since we're no longer aborting by default, while this specific test is checking that we fail on a specific input MIR so we need to trigger the error instead of falling back to SDAG.

This revision is now accepted and ready to land.Jan 2 2018, 8:30 AM
This revision was automatically updated to reflect the committed changes.