This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] ACLE: Fix issue for mismatching enum types with builtins.
ClosedPublic

Authored by sdesmalen on Apr 7 2021, 9:33 AM.

Details

Summary

This patch fixes an issue with the SVE prefetch and qinc/qdec intrinsics
that take an enum argument, but where the builtin prototype encodes
these as int. Some code in SemaDecl found the mismatch and chose
to forget about the builtin altogether, which meant that any future
code using that builtin would fail. The code that forgets about the
builtin was actually obsolete after D77491 and should have been removed.
This patch now removes that code.

This patch also fixes another issue with the SVE prefetch intrinsic
when built with C++, where the builtin didn't accept the correct
pointer type, which should be const void *.

Diff Detail

Event Timeline

sdesmalen created this revision.Apr 7 2021, 9:33 AM
sdesmalen requested review of this revision.Apr 7 2021, 9:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2021, 9:33 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

The fact that Clang chooses to (explicitly) forget about a builtin in SemaDecl.cpp was quite puzzling to me. Maybe that just shows that I don't fully understand how the builtin mechanism is supposed to work.
@rsmith and @tambre, git history showed me you have more experience in this area and touched some of that code before, so hopefully you can give me some good feedback!

tambre added a comment.Apr 9 2021, 9:55 AM

The fact that Clang chooses to (explicitly) forget about a builtin in SemaDecl.cpp was quite puzzling to me. Maybe that just shows that I don't fully understand how the builtin mechanism is supposed to work.
@rsmith and @tambre, git history showed me you have more experience in this area and touched some of that code before, so hopefully you can give me some good feedback!

Sorry for the delay, I'm lately quite busy during the week.
I'll have a look over the weekend.

tambre added inline comments.Apr 10 2021, 9:42 AM
clang/lib/Sema/SemaDecl.cpp
10968–10969

This whole block is unnecessary since D77491 and should've been removed, but seems I missed it. Reverting/forgetting builtins was problematic and the attribute-based approach solved those deficiencies.

Delete this whole block and also remove forgetBuiltin(). The builtin-related code in ActOnFunctionDeclarator() handles everything necessary. No tests break and the SVE ones will pass. :)

Removed forgetBuiltin and the code using it.

Thanks for taking time from your weekend to look into this @tambre, much appreciated!

clang/lib/Sema/SemaDecl.cpp
10968–10969

Awesome! I had indeed noticed commenting it out didn't fail any tests and wondered if removing it entirely was the way to go. Good to hear you confirming that!

tambre accepted this revision.Apr 12 2021, 12:30 AM

Thanks, LGTM! Probably worth mentioning what I had in my comment in the commit message.

This revision is now accepted and ready to land.Apr 12 2021, 12:30 AM
sdesmalen edited the summary of this revision. (Show Details)Apr 12 2021, 12:32 AM

Summary also looks good. 👍
You probably don't need to wait for more approvals.