[PATCH] D40078: [x86][icelake]VAES introduction

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Target/X86/X86.td:174
+                       "Promote selected AES instructions to AVX512/AVX registers",
+                        [FeatureAVX]>;
 def FeatureTBM     : SubtargetFeature<"tbm", "HasTBM", "true",
----------------
coby wrote:
> craig.topper wrote:
> > coby wrote:
> > > craig.topper wrote:
> > > > Should this imply FeatureAES as well?
> > > 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
> > > 
> > > 
> > 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.
> isn't it implies that only the (AES) subset implemented by VAES need to be available?
That's technically true, but a the instructions aren't enabled/disabled at that granularity.


================
Comment at: lib/Target/X86/X86InstrSSE.td:7153
 // Perform One Round of an AES Encryption/Decryption Flow
 let Predicates = [HasAVX, HasAES] in {
   defm VAESENC          : AESI_binop_rm_int<0xDC, "vaesenc",
----------------
This probably needs NoVLX_Or_NoVAES.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D40078