[PATCH] D89184: Support complex target features combinations

LiuChen3 added a comment. In D89184#2363591 , @echristo wrote: > 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. :) Thanks! :) CHANGES SINCE LAST

[PATCH] D89184: Support complex target features combinations

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. 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. :) CHANGES SINCE LAST ACTION http

[PATCH] D89184: Support complex target features combinations

LiuChen3 added a comment. In D89184#2360846 , @echristo wrote: > I'll take a look tomorrow, sorry for the delay. No problem. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89184/new/ https://reviews.llvm.org/D89184 _

[PATCH] D89184: Support complex target features combinations

echristo added a comment. I'll take a look tomorrow, sorry for the delay. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89184/new/ https://reviews.llvm.org/D89184 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/

[PATCH] D89184: Support complex target features combinations

LiuChen3 added a comment. Ping? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89184/new/ https://reviews.llvm.org/D89184 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89184: Support complex target features combinations

LiuChen3 added a comment. Hi, @echristo. What's your opinion here? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89184/new/ https://reviews.llvm.org/D89184 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin

[PATCH] D89184: Support complex target features combinations

dexonsmith added a comment. In D89184#2346453 , @pengfei wrote: > LGTM. But I suggest you waiting for 1 or 2 days to see if other reviewers > object. Given that @echristo marked this as needing changes I would suggest waiting / reaching out to confirm t

[PATCH] D89184: Support complex target features combinations

RKSimon added a comment. a few minor style comments that I noticed Comment at: clang/lib/CodeGen/CodeGenFunction.h:4705 +size_t SubexpressionStart = 0; +for (size_t i = 0; i < FeatureList.size(); ++i) { + char CurrentToken = FeatureList[i]; (style)

[PATCH] D89184: Support complex target features combinations

LiuChen3 added a comment. In D89184#2346453 , @pengfei wrote: > LGTM. But I suggest you waiting for 1 or 2 days to see if other reviewers > object. Sure. Thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89184/new/ https://reviews.llvm.org

[PATCH] D89184: Support complex target features combinations

pengfei accepted this revision. pengfei added a comment. Herald added a subscriber: dexonsmith. LGTM. But I suggest you waiting for 1 or 2 days to see if other reviewers object. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89184/new/ https://reviews.llvm.org/D89184 __

[PATCH] D89184: Support complex target features combinations

LiuChen3 added a comment. ping? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89184/new/ https://reviews.llvm.org/D89184 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89184: Support complex target features combinations

pengfei added a comment. > D89105 appears to use only `"avx512vl , > avx512vnni | avxvnni"`. > Does it mean `(avx512vl , avx512vnni) | avxvnni` or `avx512vl , (avx512vnni | > avxvnni)` ? We need to express combination to `(avx512vl , avx512vnni) | avxvnni`, th

[PATCH] D89184: Support complex target features combinations

LiuChen3 added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2389 } -if (!hasRequiredFeatures(ReqFeatures, CGM, FD, MissingFeature)) +if (!llvm::all_of(ReqFeatures, [&](StringRef Feature) { + if (!CallerFeatureMap.lookup(Feature)) {

[PATCH] D89184: Support complex target features combinations

LiuChen3 added a comment. > D89105 appears to use only `"avx512vl , > avx512vnni | avxvnni"`. > Does it mean `(avx512vl , avx512vnni) | avxvnni` or `avx512vl , (avx512vnni | > avxvnni)` ? Yes. "avx512vl , avx512vnni | avxvnni" means (avx512vl , avx512vnni) | a

[PATCH] D89184: Support complex target features combinations

echristo requested changes to this revision. echristo added a comment. This revision now requires changes to proceed. Mark as requesting changes :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89184/new/ https://reviews.llvm.org/D89184 ___ c

[PATCH] D89184: Support complex target features combinations

echristo added a comment. 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 Comment at:

[PATCH] D89184: Support complex target features combinations

tra accepted this revision. tra added a comment. This revision is now accepted and ready to land. In D89184#2328144 , @craig.topper wrote: > This is needed for D89105 and was split > from it. D89105

[PATCH] D89184: Support complex target features combinations

craig.topper added a comment. This is needed for D89105 and was split from it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89184/new/ https://reviews.llvm.org/D89184 ___ cfe-commits mailing list cfe-commit

[PATCH] D89184: Support complex target features combinations

tra requested changes to this revision. tra added a comment. This revision now requires changes to proceed. 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org

[PATCH] D89184: Support complex target features combinations

tra accepted this revision. tra added a subscriber: echristo. tra added a comment. This revision is now accepted and ready to land. @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