This is an archive of the discontinued LLVM Phabricator instance.

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:

  1. Adding support for parentheses, making the combination of target features more diverse;
  2. Making the priority of ’,‘ is higher than that of '|' by default. So I need to make some change with PTX Builtin function.

Diff Detail

Event Timeline

LiuChen3 created this revision.Oct 10 2020, 3:47 AM
LiuChen3 requested review of this revision.Oct 10 2020, 3:47 AM
tra added a comment.Oct 12 2020, 11:04 AM

Would it make sense to add a negation, too?

clang/lib/CodeGen/CodeGenFunction.h
4698

Nit: ++i

4699

I'd extract it into a variable -- you're using it in multiple places. Giving it a sensible name (current_token ?) would help readability.

4704

AndExpressionStart ?

4721–4722

Using StringRef& and modifying caller's copy is a somewhat non-obvious way to return values (or to maintain parser state).
I think explicitly returning a pair {HasFeatures, RemainingFeatures} would make it easier to understand the code.

clang/unittests/CodeGen/CheckTargetFeaturesTest.cpp
17

Is doCheck("A,B,C,D", ...) expected to work? I'd add test cases for that, too.

In D89184#2325597, @tra wrote:

Would it make sense to add a negation, too?

I think we always define a builtin function under given feature.
I don't think we need to add negation unless it does be needed on some circumstance.

clang/lib/CodeGen/CodeGenFunction.h
4699

Maybe CurrentToken to satisfy the name conversion.

4704

The expression in parentheses maybe not AND, e.g. (A|B).
Maybe SubexpressionStart is better.

clang/unittests/CodeGen/CheckTargetFeaturesTest.cpp
17

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.

In D89184#2325597, @tra wrote:

Would it make sense to add a negation, too?

Thanks for your review. I agree with @pengfei . It seems that there is no scene where A needs to be used for now.
If needed, we can add later.

clang/unittests/CodeGen/CheckTargetFeaturesTest.cpp
17

I think @tra 's mean is adding tests like ASSERT_FALSE(doCheck("A,B,C,D", "A")); .

LiuChen3 updated this revision to Diff 297769.Oct 12 2020, 11:09 PM

Address comments.

LiuChen3 marked 4 inline comments as done.Oct 12 2020, 11:17 PM
LiuChen3 added inline comments.
clang/lib/CodeGen/CodeGenFunction.h
4721–4722

Thanks for your advice. I think using "struct" may has better readability.

LGTM with nit. I'd like to see if others have objection.

clang/lib/CodeGen/CodeGenFunction.h
4699

StringRef is a small structure, we prefer passing value rather than reference.

4741

Same as above.

LiuChen3 updated this revision to Diff 297777.Oct 13 2020, 12:11 AM

Address comments

LGTM with nit. I'd like to see if others have objection.

Thanks for your review.

tra accepted this revision.Oct 13 2020, 10:21 AM
tra added a subscriber: echristo.

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

clang/unittests/CodeGen/CheckTargetFeaturesTest.cpp
17

Yes. I should've phrased it better. I meant tests w/o parens in the feature requirements.

This revision is now accepted and ready to land.Oct 13 2020, 10:21 AM
tra requested changes to this revision.Oct 13 2020, 10:24 AM

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

This is needed for D89105 and was split from it.

tra accepted this revision.Oct 13 2020, 11:17 AM

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.

This revision is now accepted and ready to land.Oct 13 2020, 11:17 AM

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

clang/lib/CodeGen/CodeGenFunction.cpp
2372–2378

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

clang/lib/CodeGen/CodeGenFunction.h
4691

Needs comments including an explanation of syntax.

clang/unittests/CodeGen/CheckTargetFeaturesTest.cpp
7

Comment the test with what you're testing.

echristo requested changes to this revision.Oct 13 2020, 1:45 PM

Mark as requesting changes :)

This revision now requires changes to proceed.Oct 13 2020, 1:45 PM

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.

LiuChen3 added inline comments.Oct 13 2020, 10:18 PM
clang/lib/CodeGen/CodeGenFunction.cpp
2372–2378

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

LiuChen3 updated this revision to Diff 298057.Oct 14 2020, 12:29 AM

Address comments.

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.

LiuChen3 updated this revision to Diff 298269.Oct 14 2020, 5:50 PM

Add a line of comment on unit test.

pengfei accepted this revision.Oct 21 2020, 11:41 PM

LGTM. But I suggest you waiting for 1 or 2 days to see if other reviewers object.

LGTM. But I suggest you waiting for 1 or 2 days to see if other reviewers object.

Sure. Thanks.

a few minor style comments that I noticed

clang/lib/CodeGen/CodeGenFunction.h
4713

(style)

for (size_t i = 0, e = FeatureList.size(); i < e; ++i) {
4721

(style)

++InParentheses;
4724
--InParentheses;
LiuChen3 updated this revision to Diff 299901.Oct 22 2020, 2:33 AM

Address comments

LiuChen3 marked 3 inline comments as done.Oct 22 2020, 2:34 AM

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.

Hi, @echristo. What's your opinion here?

I'll take a look tomorrow, sorry for the delay.

I'll take a look tomorrow, sorry for the delay.

No problem. Thanks!

echristo accepted this revision.Oct 29 2020, 6:58 PM

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

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! :)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 29 2020, 7:36 PM