Page MenuHomePhabricator

[Clang] Add clang attribute `clang_builtin_alias`.
ClosedPublic

Authored by HsiangKai on Thu, Apr 15, 6:28 PM.

Details

Summary

In some cases, we want to provide the alias name for the clang builtins.
For example, the arguments must be constant integers for some RISC-V builtins.
If we use wrapper functions, we could not constrain the arguments be constant
integer. This attribute is used to achieve the purpose.

Besides this, use clang_builtin_alias is more efficient than using
wrapper functions. We use this attribute to deal with test time issue
reported in https://bugs.llvm.org/show_bug.cgi?id=49962.

In our downstream testing, it could decrease the testing time from 6.3
seconds to 3.7 seconds for vloxei.c test.

Diff Detail

Event Timeline

HsiangKai created this revision.Thu, Apr 15, 6:28 PM
HsiangKai requested review of this revision.Thu, Apr 15, 6:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Apr 15, 6:28 PM
craig.topper edited the summary of this revision. (Show Details)Thu, Apr 15, 6:38 PM
craig.topper edited the summary of this revision. (Show Details)
craig.topper edited the summary of this revision. (Show Details)Thu, Apr 15, 6:39 PM
kito-cheng added inline comments.Thu, Apr 15, 7:48 PM
clang/test/CodeGen/RISCV/riscv-attr-builtin-alias.c
10

I guess this is not needed anymore? or at least could be reduced?

Could you also check the compiler diagnostic messages? it will report __builtin_rvv_vadd_vv_i8m1 or vadd_generic if argument type mis-match, which one you expected? I assume without __clang_riscv_builtin_alias clang will report vadd_generic?

Is this idea that this will later be "automatically" applied to builtins in riscv_vector.h?

Is this idea that this will later be "automatically" applied to builtins in riscv_vector.h?

I will prepare another patch to update the header generator.

HsiangKai updated this revision to Diff 338105.Fri, Apr 16, 7:26 AM

Add a negative test case.

HsiangKai marked an inline comment as done.Fri, Apr 16, 7:26 AM

Look good to me.
BTW, we also need to update the document later.

HsiangKai updated this revision to Diff 338411.Sun, Apr 18, 7:17 PM

Update document.

Look good to me.
BTW, we also need to update the document later.

AttributeReference.html will be updated according to the content of AttrDocs.td.

Is it okay to you? @kito-cheng @frasercrmck
This patch could fix https://bugs.llvm.org/show_bug.cgi?id=49962, I hope this could be merged soon.

frasercrmck added inline comments.Tue, Apr 20, 1:09 AM
clang/include/clang/Basic/AttrDocs.td
2141

Is vget real? I can't see it in riscv_vector.h

HsiangKai updated this revision to Diff 338824.Tue, Apr 20, 4:45 AM

Refine the document.

HsiangKai marked an inline comment as done.Tue, Apr 20, 4:46 AM
HsiangKai added inline comments.
clang/include/clang/Basic/AttrDocs.td
2141

Yes, it is for Zvlsseg. I have not upstreamed it. I use vadd instead in the description.

frasercrmck accepted this revision.Tue, Apr 20, 5:29 AM

LGTM. Anything else would be wondering if it can be merged/genericised with Arm somehow but that's not a blocker.

clang/include/clang/Basic/AttrDocs.td
2141

Ah, great. I think vadd's a good example.

This revision is now accepted and ready to land.Tue, Apr 20, 5:29 AM
aaron.ballman added inline comments.Tue, Apr 20, 5:34 AM
clang/include/clang/Basic/Attr.td
1780

Why is this being given a reserved identifier name? That's pretty atypical for attributes as attributes can optionally be prefixed and suffixed with __ already.

clang/include/clang/Basic/AttrDocs.td
2147–2150

Why a specific attribute for only one architecture rather than a more general solution that can be used for multiple architectures? We already have __clang_arm_builtin_alias (with the incorrect name, so I guess I see where you got the __ from) and now we're extending it. I am not certain if there's a reason why we should use a different attribute for each target though.

HsiangKai marked an inline comment as done.Tue, Apr 20, 8:34 AM
HsiangKai added inline comments.
clang/include/clang/Basic/AttrDocs.td
2147–2150

I agree with you. We should not define the attribute as target specific. This patch is a quick solution to address the issue https://bugs.llvm.org/show_bug.cgi?id=49962.

I will refactor this patch and try to merge it with __clang_arm_builtin_alias in some way.

HsiangKai added inline comments.Tue, Apr 20, 11:32 PM
clang/include/clang/Basic/AttrDocs.td
2147–2150

@aaron.ballman Could we land this patch first to reduce the test time?
I agree with you that it is better to have one unified attribute for the purpose. However, if we change the attribute defined by ARM, we need to negotiate with them and there may be already some use cases in their users. I am not sure the impact of the modification.

I could modify my patch to make the attribute target independent and leave ARM's attribute as before. What do you think?

aaron.ballman added inline comments.Wed, Apr 21, 10:41 AM
clang/include/clang/Basic/AttrDocs.td
2147–2150

As best I can tell, at least from the attribute that we surface for the users, there shouldn't be much difference between the RISC-V and the ARM attributes. They're both accepted on the same subjects, have the same attribute arguments, and have the same semantic effect, etc. It seems like the only difference is with semantic checking of whether the builtin is valid for the architecture. Based on the assumption that these are effectively doing the same thing, I'd rather we land the generic attribute so that we can deprecate __clang_arm_builtin_alias in favor of the general attribute. If we add __clang_riscv_builtin_alias before adding the generic attribute, it's one more thing for us to deprecate and then remove.

Note, I'd be totally fine if the generic attribute only handles ARM and RISC-V builtins initially (and error on the other architectures). We can add support for other architectures as the need arises.

Do you think that approach makes sense?

HsiangKai added inline comments.Wed, Apr 21, 3:24 PM
clang/include/clang/Basic/AttrDocs.td
2147–2150

It makes sense. I will integrate D100930 into this one.

HsiangKai updated this revision to Diff 339602.Thu, Apr 22, 6:28 AM

Add clang attribute clang_builtin_alias.

HsiangKai retitled this revision from [RISCV] Add new attribute __clang_riscv_builtin_alias for intrinsics. to [Clang] Add clang attribute `clang_builtin_alias`..Thu, Apr 22, 6:28 AM
HsiangKai edited the summary of this revision. (Show Details)
HsiangKai updated this revision to Diff 339606.Thu, Apr 22, 6:44 AM

Update test case.

@aaron.ballman, could you help me to review it again? Thanks.

aaron.ballman added inline comments.Fri, Apr 23, 9:27 AM
clang/include/clang/Basic/Attr.td
646

Hrm, this is interesting -- this will allow the user to write __attribute__((clang_builtin_alias(...))) which is nice, but it also allows [[clang::clang_builtin_alias(...)]] which duplicates the clang in that spelling and feels a bit awkward.

While we could steal __attribute__((builtin_alias)) as it seems like no one is using it, that feels a bit heavy-handed as builtins may not be portable across compilers.

Based on that, I sort of think we should go with:

let Spellings = [CXX11<"clang", "builtin_alias">, C2x<"clang", "builtin_alias">, GNU<"clang_builtin_alias">];

What do you think?

clang/include/clang/Basic/AttrDocs.td
4702

Can you also update ArmBuiltinAliasDocs to point users to the more general attribute?

clang/include/clang/Basic/DiagnosticSemaKinds.td
4030
clang/lib/Sema/SemaDeclAttr.cpp
5181
HsiangKai added inline comments.Fri, Apr 23, 8:56 PM
clang/include/clang/Basic/Attr.td
646

It makes sense to me.

HsiangKai updated this revision to Diff 340232.Fri, Apr 23, 8:57 PM

Address comments.

HsiangKai marked 3 inline comments as done.Fri, Apr 23, 8:58 PM
HsiangKai marked an inline comment as done.
aaron.ballman accepted this revision.Sat, Apr 24, 9:12 AM

LGTM aside from a small nit with the documentation wording.

clang/include/clang/Basic/AttrDocs.td
5716

I'd like to avoid saying "deprecated" because then people may expect to see warnings about use of the ARM attribute. I don't know if the ARM folks are ready for it to be officially deprecated as opposed to just discouraged.

This revision was landed with ongoing or failed builds.Sat, Apr 24, 5:49 PM
This revision was automatically updated to reflect the committed changes.