Page MenuHomePhabricator

[x86][icelake]VAES introduction
ClosedPublic

Authored by coby on Nov 15 2017, 7:29 AM.

Details

Summary

an icelake promotion of AES

Diff Detail

Repository
rL LLVM

Event Timeline

coby created this revision.Nov 15 2017, 7:29 AM

intel syntax encoding test is to be added shortly

craig.topper added inline comments.Nov 15 2017, 10:02 AM
lib/Support/Host.cpp
433 ↗(On Diff #123026)

This isn't needed if it isn't used by code in getHostCPUName

1063 ↗(On Diff #123026)

This isn't needed if it isn't used by code in getHostCPUName.

1481 ↗(On Diff #123026)

Please use ECX >> 9 or whatever the correct bit is like the rest of the code.

Qualify with HasAVXSave as well.

lib/Target/X86/X86.td
174 ↗(On Diff #123026)

Should this imply FeatureAES as well?

lib/Target/X86/X86InstrInfo.td
829 ↗(On Diff #123026)

I don't think you nee the assembler predicate. We only need it on the core AVX512 features because it enables the parser to recognize mask registers and embeded rounding. In general we don't do fine grained control of what instructinos are available to the assembler.

coby added inline comments.Nov 15 2017, 11:33 AM
lib/Target/X86/X86.td
174 ↗(On Diff #123026)

not sure,
it has no direct dependency afaiu, like AVX, for example, i.e. it does not imply that AES insns are apparent.
practically, if you want to use 'vaesenc', for example, you ought to have a Key, so you'll probably want to use 'aeskeygenassist' which is on AES solely, but than again - it (vaes) may still be provided on its own, so I find it yet questionable

coby marked 3 inline comments as done.Nov 15 2017, 11:48 AM
coby added inline comments.
lib/Support/Host.cpp
433 ↗(On Diff #123026)

I see, thanks.
I assume same logic is to be applied to entries like FEATURE_PCLMUL?

lib/Target/X86/X86InstrInfo.td
829 ↗(On Diff #123026)

verified it to be right. thanks. will remove

coby marked an inline comment as done.Nov 15 2017, 11:58 AM
coby updated this revision to Diff 123071.Nov 15 2017, 12:33 PM

addressed Craig's comments

craig.topper added inline comments.Nov 15 2017, 12:50 PM
lib/Support/Host.cpp
433 ↗(On Diff #123026)

The first 31 values need to be kept in sync with compiler-rt since we have a copy and paste of some of this code in compiler-rt. For runtime CPU detection in compiled binaries. It makes diffing easier if they are synchronized. I just committed some comments that clarify that a little better. I'm hoping to move this enum to a header file that clang can use as a proxy for what's in compiler-rt as well since clang can't include any files from the compiler-rt project. Currently we just have a separate copy code in clang too.

lib/Target/X86/X86.td
174 ↗(On Diff #123026)

The EVEX->VEX pass is going to turn 128-bit EVEX encoded AES instructions into VEX encoded versions. So those instructions better be available with VAES.

coby added inline comments.Nov 15 2017, 1:29 PM
lib/Support/Host.cpp
433 ↗(On Diff #123026)

I see. thanks for the elaborated explanation!

lib/Target/X86/X86.td
174 ↗(On Diff #123026)

isn't it implies that only the (AES) subset implemented by VAES need to be available?

craig.topper added inline comments.Nov 15 2017, 5:14 PM
lib/Target/X86/X86.td
174 ↗(On Diff #123026)

That's technically true, but a the instructions aren't enabled/disabled at that granularity.

lib/Target/X86/X86InstrSSE.td
7153 ↗(On Diff #123071)

This probably needs NoVLX_Or_NoVAES.

The fact that hasVAES doesn't imply HasAES is hiding this bug unlike the PCLMUL patch.

coby updated this revision to Diff 123569.Nov 20 2017, 5:06 AM
coby updated this revision to Diff 123570.Nov 20 2017, 5:28 AM

further refined selection of conflicting insns, as suggested by Craig (thanks)

This revision is now accepted and ready to land.Nov 20 2017, 7:47 AM
This revision was automatically updated to reflect the committed changes.