This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Fix LSE2/LSE128/RCPC3 precedence
ClosedPublic

Authored by tmatheson on Feb 7 2023, 8:36 AM.

Details

Summary

D142712 added tests for when both lse2 and lse128 are available, but
in practice there is no way to enable LSE128 without LSE2 from clang:
LSE128 is a v9 only feature and LSE2 has been mandatory since v8.4,
and +/-lse2 can not be specified on the clang command line.

Therefore it makes more sense that lse2+lse128 should emit lse128
instructions, otherwise they will not be emitted at all.

It also makes sense to remove the lse128-only backend tests if that set
of attributes is never set by the frontend.

Diff Detail

Event Timeline

tmatheson created this revision.Feb 7 2023, 8:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2023, 8:36 AM
tmatheson requested review of this revision.Feb 7 2023, 8:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2023, 8:36 AM

Broadly, I think this is clearer, and shows that RCPC3 is more specific than LSE128 which is more specific than LSE2. I do have one concern shown in the tests.

llvm/test/CodeGen/AArch64/Atomics/aarch64-atomic-store-lse2_lse128.ll
127

I think in this case the stp is better, given it won't overwrite x0 and x1 - this doesn't matter if the values in both are dead at this point, but if they're used by anything after the store, then with swpp they will need to be copied into a different register.

tmatheson updated this revision to Diff 495597.Feb 7 2023, 10:42 AM

Use LSE2 for unordered/monotonic

lenary accepted this revision.Feb 7 2023, 12:39 PM
This revision is now accepted and ready to land.Feb 7 2023, 12:39 PM
This revision was landed with ongoing or failed builds.Feb 13 2023, 4:32 AM
This revision was automatically updated to reflect the committed changes.
tmatheson marked an inline comment as done.
llvm/test/CodeGen/AArch64/Atomics/aarch64_be-atomic-store-lse2_lse128.ll