[PATCH] D30451: [X86][AVX512] Adding new LLVM TableGen backend which generates the EVEX2VEX compressing tables.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: utils/TableGen/X86EVEX2VEXTablesEmitter.cpp:68
+  const vector<string> ExceptionList = {
+      "VCVTQQ2PD",
+      "VCVTQQ2PS",
----------------
aymanmus wrote:
> craig.topper wrote:
> > Why is this a vector and not just a static table of strings?
> > 
> > Can you add a comment indicating what each of these corresponds to in VEX just so if someone is curious they easily find it?
> Any table defined inside the class would require stating the size when defined (ExceptionList[11]), So I figured it's more maintainable this way.
> The mappings of these instructions to VEX instructions make no sense, there is no "meaning" to the relation.
Well from a quick look it seemed like some of them were just Q versions of similar instructions in VEX that have WIG right? Is there not some way we can detect that these have W==1 and infer that we shouldn't use a VEX WIG instruction?


================
Comment at: utils/TableGen/X86EVEX2VEXTablesEmitter.cpp:20
+
+#include <iostream>
+using namespace std;
----------------
I think LLVM coding standards say not to use iostream.


================
Comment at: utils/TableGen/X86EVEX2VEXTablesEmitter.cpp:21
+#include <iostream>
+using namespace std;
+using namespace llvm;
----------------
using namespace std is frowned upon in LLVM. Please use the std:: prefix explicitly where needed.


================
Comment at: utils/TableGen/X86EVEX2VEXTablesEmitter.cpp:84
+    for (const string InstStr : ExceptionList) {
+      if (Inst->TheDef->getName().find(InstStr) == 0)
+        return true;
----------------
Can we use StringRef::startswith here?


================
Comment at: utils/TableGen/X86EVEX2VEXTablesEmitter.cpp:248
+      } else if (isMemoryOperand(OpRec1) && isMemoryOperand(OpRec2)) {
+        if (OpRec1 != OpRec2)
+          return false;
----------------
This OpRec != OpRec2 check seems redundant due to the earlier check and continue.


================
Comment at: utils/TableGen/X86EVEX2VEXTablesEmitter.cpp:305
+    // Add relevant EVEX encoded instructions to EVEXInsts
+    else if (Inst->TheDef->getValueAsDef("OpEnc")->getName().str() ==
+                 "EncEVEX" &&
----------------
You shouldn't need to call str(). getName() should return a StringRef that has an operator==


================
Comment at: utils/TableGen/X86EVEX2VEXTablesEmitter.cpp:309
+             !Inst->TheDef->getValueAsBit("hasEVEX_B") &&
+             !inExceptionList(Inst))
+      EVEXInsts.push_back(Inst);
----------------
Should we skip EVEX_LL == 2 here?


================
Comment at: utils/TableGen/X86EVEX2VEXTablesEmitter.cpp:318
+    // (instructions with the same opcode) using function object IsMatch.
+    auto Match = find_if(VEXInsts[Opcode].begin(), VEXInsts[Opcode].end(),
+                                                          IsMatch(EVEXInst));
----------------
I think this is calling std::find_if, but we should have an llvm::find_if that can take an object that supports with begin() and end() without needing to call them explicitly. So its two parameters instead of three.


https://reviews.llvm.org/D30451