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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Support/Host.cpp:433
   FEATURE_SHA,
+  FEATURE_VAES,
 };
----------------
This isn't needed if it isn't used by code in getHostCPUName


================
Comment at: lib/Support/Host.cpp:1063
     Features |= 1 << FEATURE_AVX512VPOPCNTDQ;
+  if (HasLeaf7 && (ECX & 512))
+    Features2 |= 1 << (FEATURE_VAES - 32);
----------------
This isn't needed if it isn't used by code in getHostCPUName.


================
Comment at: lib/Support/Host.cpp:1481
   Features["pku"] = HasLeaf7 && ((ECX >> 4) & 1);
+  Features["vaes"] = HasLeaf7 && (ECX & 512);
 
----------------
Please use ECX >> 9 or whatever the correct bit is like the rest of the code.

Qualify with HasAVXSave as well.


================
Comment at: lib/Target/X86/X86.td:174
+                       "Promote selected AES instructions to AVX512/AVX registers",
+                        [FeatureAVX]>;
 def FeatureTBM     : SubtargetFeature<"tbm", "HasTBM", "true",
----------------
Should this imply FeatureAES as well?


================
Comment at: lib/Target/X86/X86InstrInfo.td:829
+def HasVAES      : Predicate<"Subtarget->hasVAES()">,
+                    AssemblerPredicate<"FeatureVAES",
+                      "AES instructions on AVX512 registers">;
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D40078