[PATCH] D98011: [X86][NFC] 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/X86EvexToVex.cpp:253-254
   ArrayRef<X86EvexToVexCompressTableEntry> Table =
-    (Desc.TSFlags & X86II::VEX_L) ? makeArrayRef(X86EvexToVex256CompressTable)
-                                  : makeArrayRef(X86EvexToVex128CompressTable);
+      (Desc.TSFlags & X86II::VEX_L)
+          ? makeArrayRef(X86EvexToVex256CompressTable)
+          : makeArrayRef(X86EvexToVex128CompressTable);
----------------
Don't need to reformat the unrelated code.


================
Comment at: llvm/lib/Target/X86/X86InstrFormats.td:239
 class SIMD_EXC { list<Register> Uses = [MXCSR]; bit mayRaiseFPException = 1; }
+class CheckPredicate { bit checkPredicate = 1; }
 
----------------
This is not needed.


================
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?
 
----------------
LiuChen3 wrote:
> pengfei wrote:
> > checkVEXPredicate?
> Maybe in the future we need check more Predicate than VEX?
I think you can add VEX for now since you mentioned on the comment too.


================
Comment at: llvm/lib/Target/X86/X86InstrSSE.td:7204
 
-defm VPDPBUSD   : avx_vnni_rm<0x50, "vpdpbusd", X86Vpdpbusd, 0>, ExplicitVEXPrefix;
-defm VPDPBUSDS  : avx_vnni_rm<0x51, "vpdpbusds", X86Vpdpbusds, 0>, ExplicitVEXPrefix;
-defm VPDPWSSD   : avx_vnni_rm<0x52, "vpdpwssd",  X86Vpdpwssd, 1>, ExplicitVEXPrefix;
-defm VPDPWSSDS  : avx_vnni_rm<0x53, "vpdpwssds", X86Vpdpwssds, 1>, ExplicitVEXPrefix;
+defm VPDPBUSD   : avx_vnni_rm<0x50, "vpdpbusd", X86Vpdpbusd, 0>, ExplicitVEXPrefix, CheckPredicate;
+defm VPDPBUSDS  : avx_vnni_rm<0x51, "vpdpbusds", X86Vpdpbusds, 0>, ExplicitVEXPrefix, CheckPredicate;
----------------
These are not needed.


================
Comment at: llvm/utils/TableGen/X86EVEX2VEXTablesEmitter.cpp:199-200
+      if (PredicatesRecordsName.startswith("HasAVX")) {
+        if (!Predicates.empty())
+          Predicates += " && ";
+        Predicates +=
----------------
Do you have cases that have more than one `HasAVXxxx` in Predicates?


================
Comment at: llvm/utils/TableGen/X86EVEX2VEXTablesEmitter.cpp:202
+        Predicates +=
+            '(' + PredicatesRecords[i]->getValueAsString("CondString").str() +
+            ')';
----------------
Not necessary?


================
Comment at: llvm/utils/TableGen/X86EVEX2VEXTablesEmitter.cpp:206-207
+    }
+    if (Predicates.empty())
+      Predicates = "true";
+    return Predicates;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98011