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

[Pengfei Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/Target/X86/X86InstrFormats.td:356
   bit ExplicitVEXPrefix = 0; // Force the instruction to use VEX encoding.
+  bit checkPredicate = 0; // Does this VEX inst should check predicate?
 
----------------
checkVEXPredicate?


================
Comment at: llvm/lib/Target/X86/X86InstrSSE.td:7169
 //===----------------------------------------------------------------------===//
 let Predicates = [HasAVXVNNI, NoVLX_Or_NoVNNI], Constraints = "$src1 = $dst" in
 multiclass avx_vnni_rm<bits<8> opc, string OpcodeStr, SDNode OpNode,
----------------
Should be simple to add here i.e. `ExplicitVEXPrefix, checkPredicate = 1`


================
Comment at: llvm/utils/TableGen/X86EVEX2VEXTablesEmitter.cpp:23
+// List of the Predicates should be ignored.
+StringRef ExcludePredicates[] = {"OptForSize", "OptForSpeed", "NoAVX512",
+                                 "NoFMA4",     "NoBWI",       "NoVLX",
----------------
Can we assume the predicates that need to check all started with `HasAVXxxx`. Then we don't need such table.


================
Comment at: llvm/utils/TableGen/X86EVEX2VEXTablesEmitter.cpp:44
+  // Represent predicates of VEX instructions.
+  std::vector<Predicate> EVEX2VEX128Predicates;
+  std::vector<Predicate> EVEX2VEX256Predicates;
----------------
Do we really need 2 tables?


================
Comment at: llvm/utils/TableGen/X86EVEX2VEXTablesEmitter.cpp:202
+        Inst->TheDef->getValueAsListOfDefs("Predicates");
+    for (unsigned i = 0, e = PredicatesRecords.size(); i != e; ++i) {
+      StringRef PredicatesRecordsName = PredicatesRecords[i]->getName();
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98011/new/

https://reviews.llvm.org/D98011