This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Use LL for 64-bit arguments
ClosedPublic

Authored by samparker on Jan 17 2019, 6:28 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

samparker created this revision.Jan 17 2019, 6:28 AM
TomTan added a subscriber: TomTan.Jan 17 2019, 10:54 AM
rnk added a subscriber: rnk.Jan 17 2019, 11:27 AM
rnk added inline comments.
include/clang/Basic/BuiltinsAArch64.def
54 ↗(On Diff #182268)

If we can't change the signature on Linux (I don't see any reason why couldn't, though) we can use the 'W' builtin code to indicate that it's long on LP64 and long long on LLP64.

While you're here, can you also fix __builtin_arm_rbit64, __builtin_arm_rsr64, and __builtin_arm_wsr64?

include/clang/Basic/BuiltinsAArch64.def
54 ↗(On Diff #182268)

Changing the signature would be fine, I think (we don't expect people to use these directly, and even if they did it wouldn't make any practical difference outside of obscure edge cases). But probably better to match the ACLE signature for the sake of clarity.

samparker updated this revision to Diff 182481.Jan 18 2019, 2:10 AM

Updated wsr, rsr and rbit

samparker retitled this revision from [AArch64] Use LLU for 64-bit crc32 arguments to [AArch64] Use LL for 64-bit arguments.Jan 18 2019, 2:11 AM
samparker edited the summary of this revision. (Show Details)
efriedma accepted this revision.Jan 18 2019, 10:56 AM

LGTM

test/CodeGen/builtins-arm64.c
2 ↗(On Diff #182481)

"-S" is redundant.

This revision is now accepted and ready to land.Jan 18 2019, 10:56 AM
This revision was automatically updated to reflect the committed changes.
phosek added a subscriber: phosek.Jan 24 2019, 5:16 PM

This broke our kernel build which uses PRIx64 to print the return value of __builtin_arm_rsr64. This is correct because according to ACLE, the return type of that builtin should be uint64_t. Problem is that on AArch64, unsigned long is equivalent to uint64_t, not unsigned long long (at least on Linux and other platforms like Fuchsia).

The reason this wasn't caught by the test is because that test isn't strict enough. I've updated the test in D57210. I believe this change should be reverted because it's wrong and I created revert in D57209. I think the correct solution might be what rnk suggested in his comment, that is to use W instead of LL. I plan on landing my revert if I don't hear back by EOD because this is currently blocking our kernel build with the latest Clang.

samparker reopened this revision.Jan 28 2019, 3:36 AM
This revision is now accepted and ready to land.Jan 28 2019, 3:36 AM
samparker updated this revision to Diff 183820.Jan 28 2019, 3:38 AM
samparker added reviewers: phosek, rnk.

Changed the builtins to use W instead of LL. I've also updated the tests, adding a test for rbitl.

This revision was automatically updated to reflect the committed changes.