[PATCH] D89184: Support complex target features combinations

[LiuChen via Phabricator via cfe-commits]

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 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

[Eric Christopher via Phabricator via cfe-commits]

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
  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

[LiuChen via Phabricator via cfe-commits]

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

___
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

[Eric Christopher via Phabricator via cfe-commits]

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/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89184: Support complex target features combinations

[LiuChen via Phabricator via cfe-commits]

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

[LiuChen via Phabricator via cfe-commits]

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/mailman/listinfo/cfe-commits

[PATCH] D89184: Support complex target features combinations

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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 the concerns were addressed.


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

[Simon Pilgrim via Phabricator via cfe-commits]

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)
```
for (size_t i = 0, e = FeatureList.size(); i < e; ++i) {
```



Comment at: clang/lib/CodeGen/CodeGenFunction.h:4713
+  SubexpressionStart = i + 1;
+InParentheses++;
+break;

(style)
```
++InParentheses;
```



Comment at: clang/lib/CodeGen/CodeGenFunction.h:4716
+  case ')':
+InParentheses--;
+assert(InParentheses >= 0 && "Parentheses are not in pair");

```
--InParentheses;
```




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

[LiuChen via Phabricator via cfe-commits]

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/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 Wang via Phabricator via cfe-commits]

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

___
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

[LiuChen via Phabricator via cfe-commits]

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 Wang via Phabricator via cfe-commits]

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`, the 
previous code will turn it into `avx512vl , (avx512vnni | avxvnni)`.
With this patch, `"avx512vl , avx512vnni | avxvnni"` will turn into `(avx512vl 
, avx512vnni) | avxvnni` by always prioritizing `","`.
I agreed with @echristo that we do need to add some comments for that.


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

[LiuChen via Phabricator via cfe-commits]

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)) {

echristo wrote:
> Not sure why the change here. It'd be good to be able to reuse the same code 
> here. What's up?
Since here is no need for complex feature processing, just a simple handle here.
Besides, 'hasRequiredFeatures' function has different input argument and does 
not handle 'MissingFeature'. 



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

[LiuChen via Phabricator via cfe-commits]

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) | avxvnni. 
This is the reason why we need this patch.


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

[Eric Christopher via Phabricator via cfe-commits]

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

___
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

[Eric Christopher via Phabricator via cfe-commits]

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: clang/lib/CodeGen/CodeGenFunction.cpp:2389
 }
-if (!hasRequiredFeatures(ReqFeatures, CGM, FD, MissingFeature))
+if (!llvm::all_of(ReqFeatures, [&](StringRef Feature) {
+  if (!CallerFeatureMap.lookup(Feature)) {

Not sure why the change here. It'd be good to be able to reuse the same code 
here. What's up?



Comment at: clang/lib/CodeGen/CodeGenFunction.h:4683
 
+class TargetFeatures {
+  struct FeatureListStatus {

Needs comments including an explanation of syntax.



Comment at: clang/unittests/CodeGen/CheckTargetFeaturesTest.cpp:6
+
+TEST(CheckTargetFeaturesTest, checkBuiltinFeatures) {
+  auto doCheck = [](StringRef BuiltinFeatures, StringRef FuncFeatures) {

Comment the test with what you're testing.


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

[Artem Belevich via Phabricator via cfe-commits]

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   appears to use only `"avx512vl , 
avx512vnni | avxvnni"`.
Does it mean `(avx512vl , avx512vnni) | avxvnni` or `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.
Caveat: I'm not the owner of CodeGen, so you may want to give someone familiar 
with it a chance to chime in before landing the change.


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

[Craig Topper via Phabricator via cfe-commits]

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-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89184: Support complex target features combinations

[Artem Belevich via Phabricator via cfe-commits]

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/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

[Artem Belevich via Phabricator via cfe-commits]

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 and use for NVPTX goes. No opinion on whether it 
**should** go into codegen.
While the change appears to be sensible on general principles, I'm not aware of 
any specific case where it's actually needed.
It would help if you could elaborate on why you need this functionality.




Comment at: clang/unittests/CodeGen/CheckTargetFeaturesTest.cpp:16
+  };
+  ASSERT_TRUE(doCheck("A|B,C|D", "A"));
+  ASSERT_FALSE(doCheck("(A|B),(C|D)", "A"));

LiuChen3 wrote:
> pengfei wrote:
> > tra wrote:
> > > Is `doCheck("A,B,C,D", ...)`  expected to work? I'd add test cases for 
> > > that, too.
> > > 
> > > 
> > I may not get your point here.
> > We added the unittest to check the correctness for the function. It's an 
> > easier and more flexible way checking for complex functions than using llc.
> > I don't know if there's criteria for the unittests.
> I think @tra 's mean is adding tests like ASSERT_FALSE(doCheck("A,B,C,D", 
> "A")); .
Yes. I should've phrased it better.  I meant tests w/o parens in the feature 
requirements.


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