This patch is mainly doing two things:
- Adding support for parentheses, making the combination of target features more diverse;
- Making the priority of ’,‘ is higher than that of '|' by default. So I need to make some change with PTX Builtin function.
Paths
| Differential D89184
Support complex target features combinations ClosedPublic Authored by LiuChen3 on Oct 10 2020, 3:47 AM.
Details Summary This patch is mainly doing two things:
Diff Detail
Event TimelineComment Actions Would it make sense to add a negation, too?
Comment Actions
I think we always define a builtin function under given feature.
Comment Actions
Thanks for your review. I agree with @pengfei . It seems that there is no scene where A needs to be used for now.
Comment Actions
Thanks for your review. Comment Actions @echristo : FYI, just in case you have an opinion on bringing required feature constraint checks one step closer to being Turing-complete. :-) LGTM as far as syntax and use for NVPTX goes. No opinion on whether it should go into codegen.
This revision is now accepted and ready to land.Oct 13 2020, 10:21 AM Comment Actions Sorry, I didn't mean to stamp the change as LGTM overall yet. Can't find a way to un-LGTM, so marking it as Request Changes for now. This revision now requires changes to proceed.Oct 13 2020, 10:24 AM Comment Actions
D89105 appears to use only "avx512vl , avx512vnni | avxvnni". This change would be needed for the former, but not the latter. I assume that it's the former. If that's the case, LGTM. This revision is now accepted and ready to land.Oct 13 2020, 11:17 AM Comment Actions Hi Peng, Looks interesting, but I think the language support needs some more comments about what's expected and in addition through everything else please? I've added some inline comments with places I think could use it for sure. -eric
This revision now requires changes to proceed.Oct 13 2020, 1:45 PM Comment Actions
Yes. "avx512vl , avx512vnni | avxvnni" means (avx512vl , avx512vnni) | avxvnni. This is the reason why we need this patch.
Comment Actions
We need to express combination to (avx512vl , avx512vnni) | avxvnni, the previous code will turn it into avx512vl , (avx512vnni | avxvnni). Comment Actions
Sure. Thanks. Comment Actions
Given that @echristo marked this as needing changes I would suggest waiting / reaching out to confirm the concerns were addressed. Comment Actions
No problem. Thanks! Comment Actions Let's go ahead and unblock you, but getting a lot of this refactored would be great if you can. I think it's hitting the limits of the original design. :) This revision is now accepted and ready to land.Oct 29 2020, 6:58 PM Comment Actions
Thanks! :) Closed by commit rG00090a2b826a: Support complex target features combinations (authored by LiuChen3). · Explain WhyOct 29 2020, 7:36 PM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 301802 clang/include/clang/Basic/BuiltinsNVPTX.def
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/CodeGen/CodeGenFunction.cpp
clang/test/CodeGen/builtins-nvptx-mma.cu
clang/test/CodeGen/builtins-nvptx-sm_70.cu
clang/unittests/CodeGen/CMakeLists.txt
clang/unittests/CodeGen/CheckTargetFeaturesTest.cpp
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Needs comments including an explanation of syntax.