Page MenuHomePhabricator

[X86][NFC] Adding one flag to imply whether the instruction should check the predicate when compress EVEX instructions to VEX encoding.
ClosedPublic

Authored by LiuChen3 on Mar 4 2021, 11:59 PM.

Details

Summary

Some EVEX instructions should check the predicates when compress to VEX
encoding. For example, avx512vnni instructions. This is because avx512vnni
doesn't mean that avxvnni is supported on the target.
This patch moving the manually added check to .inc that generated by tablegen.

Diff Detail

Event Timeline

LiuChen3 created this revision.Mar 4 2021, 11:59 PM
LiuChen3 requested review of this revision.Mar 4 2021, 11:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2021, 11:59 PM

Tests, and some words in description/patch, are missing.

pengfei added inline comments.Mar 5 2021, 7:34 AM
llvm/lib/Target/X86/X86InstrFormats.td
355

checkVEXPredicate?

llvm/lib/Target/X86/X86InstrSSE.td
7169–7170

Should be simple to add here i.e. ExplicitVEXPrefix, checkPredicate = 1

llvm/utils/TableGen/X86EVEX2VEXTablesEmitter.cpp
23

Can we assume the predicates that need to check all started with HasAVXxxx. Then we don't need such table.

39

Do we really need 2 tables?

195

If we can assume the predicates that need to check all started with HasAVXxxx, then we just need to find it in the PredicatesRecords and check it only.

LuoYuanke added inline comments.Mar 5 2021, 4:32 PM
llvm/lib/Target/X86/X86InstrSSE.td
7204–7207

Do we support customized code for CheckPredicate, so that we can extend the functionality someday? In this case the CheckPredicate is {ST->hasAVXVNNI();}.

craig.topper added inline comments.Mar 5 2021, 9:57 PM
llvm/utils/TableGen/X86EVEX2VEXTablesEmitter.cpp
197

releated -> related

LiuChen3 added inline comments.Mar 7 2021, 5:25 PM
llvm/lib/Target/X86/X86InstrFormats.td
355

Maybe in the future we need check more Predicate than VEX?

llvm/lib/Target/X86/X86InstrSSE.td
7169–7170

Of course we can do this.

7204–7207

I think if you want to add customized check, you can add Predicate like "def HasAVXVNNI". Than this predicate can include in the table EVEX2VEX[128/256]Predicates.

llvm/utils/TableGen/X86EVEX2VEXTablesEmitter.cpp
23

Yes. It's good idea.

39

You are right. We don't need 2 tables.

LiuChen3 added inline comments.Mar 7 2021, 5:42 PM
llvm/utils/TableGen/X86EVEX2VEXTablesEmitter.cpp
195

Related to Yuanke's question, if support customized code for CheckPredicate, the name could be something else, so that we can't depend on the whether the predicate starts with "HasAVX". This is also my first thought. What's your opinion here? Of course we can apply your method firstly and added more conditions in the future.

pengfei added inline comments.Mar 7 2021, 6:01 PM
llvm/utils/TableGen/X86EVEX2VEXTablesEmitter.cpp
195

I'm not sure what's customized for in future. I can see its only required during this EVEX to VEX transformation. Any thoughts @LuoYuanke ?

Tests, and some words in description/patch, are missing.

Thanks for your review, @lebedev.ri. This is actually one NFC patch. I think we don't need add new tests.

LuoYuanke added inline comments.Mar 7 2021, 6:47 PM
llvm/utils/TableGen/X86EVEX2VEXTablesEmitter.cpp
195

I'm just thinking if the predict can be extensible. I agree we can apply current approach first and refactor it if any extension is needed.

LiuChen3 updated this revision to Diff 328922.Mar 7 2021, 7:28 PM

Rebase and address Pengfei and Craig's comments.

LiuChen3 retitled this revision from [X86] Adding one flag to imply whether the instruction should check the predicate when compress EVEX instructions to VEX encoding. to [X86][NFC] Adding one flag to imply whether the instruction should check the predicate when compress EVEX instructions to VEX encoding..Mar 7 2021, 7:33 PM
LiuChen3 marked 3 inline comments as done.
LiuChen3 added inline comments.
llvm/lib/Target/X86/X86InstrSSE.td
7169–7170

I am sorry that I forgot this!

LiuChen3 updated this revision to Diff 328924.Mar 7 2021, 7:40 PM

Address pengfei's comment.

pengfei added inline comments.Mar 7 2021, 8:06 PM
llvm/lib/Target/X86/X86EvexToVex.cpp
254–255

Don't need to reformat the unrelated code.

llvm/lib/Target/X86/X86InstrFormats.td
239

This is not needed.

355

I think you can add VEX for now since you mentioned on the comment too.

llvm/lib/Target/X86/X86InstrSSE.td
7204–7207

These are not needed.

llvm/utils/TableGen/X86EVEX2VEXTablesEmitter.cpp
199–200

Do you have cases that have more than one HasAVXxxx in Predicates?

202

Not necessary?

206–207

We don't need to add case XXX return ture since we use the default: return true;.
And since we always need to check predicate when we add it in the td file, we don't expect the Predicates is empty. I think we can add a assert for this case.

LiuChen3 added inline comments.Mar 8 2021, 12:10 AM
llvm/utils/TableGen/X86EVEX2VEXTablesEmitter.cpp
206–207

Yes. The default value should be 'llvm_unreachable'. In this case, "return true" is needed.

LiuChen3 updated this revision to Diff 328966.Mar 8 2021, 3:46 AM

Address comments

pengfei accepted this revision.Mar 8 2021, 4:51 AM

LGTM.

llvm/lib/Target/X86/X86InstrFormats.td
355

Nit: Not a good sentences. How about:
Force to check predicate before using VEX encoding.

llvm/utils/TableGen/X86EVEX2VEXTablesEmitter.cpp
52

Nit: Add comments like above.

195

Nit: Maybe bettter to move the comments above the for or add curly brackets for for.

This revision is now accepted and ready to land.Mar 8 2021, 4:51 AM

Tests, and some words in description/patch, are missing.

Thanks for your review, @lebedev.ri. This is actually one NFC patch. I think we don't need add new tests.

How is it NFC?

Tests, and some words in description/patch, are missing.

Thanks for your review, @lebedev.ri. This is actually one NFC patch. I think we don't need add new tests.

How is it NFC?

I think @LiuChen3 's NFC firstly means to LLVM. I.e. Moving the manually added check to .inc that generated by tablegen. It's hard to say NFC for the tablegen code. But with a coarse search, I didn't find we have a test for this EVEX2VEX backend. So I assumed it is covered by LLVM tests. By this mean, I think we can call it a NFC :)

Tests, and some words in description/patch, are missing.

Thanks for your review, @lebedev.ri. This is actually one NFC patch. I think we don't need add new tests.

How is it NFC?

I think @LiuChen3 's NFC firstly means to LLVM. I.e. Moving the manually added check to .inc that generated by tablegen. It's hard to say NFC for the tablegen code. But with a coarse search, I didn't find we have a test for this EVEX2VEX backend. So I assumed it is covered by LLVM tests. By this mean, I think we can call it a NFC :)

That should be explained in patch's description :)

Tests, and some words in description/patch, are missing.

Thanks for your review, @lebedev.ri. This is actually one NFC patch. I think we don't need add new tests.

How is it NFC?

I think @LiuChen3 's NFC firstly means to LLVM. I.e. Moving the manually added check to .inc that generated by tablegen. It's hard to say NFC for the tablegen code. But with a coarse search, I didn't find we have a test for this EVEX2VEX backend. So I assumed it is covered by LLVM tests. By this mean, I think we can call it a NFC :)

That should be explained in patch's description :)

Agreed. Thanks.

LiuChen3 marked an inline comment as done.Mar 8 2021, 4:43 PM

Tests, and some words in description/patch, are missing.

Thanks for your review, @lebedev.ri. This is actually one NFC patch. I think we don't need add new tests.

How is it NFC?

I think @LiuChen3 's NFC firstly means to LLVM. I.e. Moving the manually added check to .inc that generated by tablegen. It's hard to say NFC for the tablegen code. But with a coarse search, I didn't find we have a test for this EVEX2VEX backend. So I assumed it is covered by LLVM tests. By this mean, I think we can call it a NFC :)

That should be explained in patch's description :)

Agreed. Thanks.

It's my fault. Thanks for all of you.

LiuChen3 updated this revision to Diff 329167.Mar 8 2021, 5:08 PM

Address pengfei's comments

LiuChen3 edited the summary of this revision. (Show Details)Mar 8 2021, 5:11 PM
This revision was landed with ongoing or failed builds.Mar 9 2021, 3:58 AM
This revision was automatically updated to reflect the committed changes.