[PATCH] D32684: [X86] Adding new LLVM TableGen backend that generates the X86 backend memory folding tables.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: utils/TableGen/X86FoldTablesEmitter.cpp:69
+      else
+        result.erase(result.length() - 3, 3);
+
----------------
RKSimon wrote:
> Is there a better way to avoid having to strip the trailing " | "? If not, at least comment what this is doing.
Couldn't we just always emit a 0 at the end? We do this in other generated tables already.


================
Comment at: utils/TableGen/X86FoldTablesEmitter.cpp:110
+  // List of instructions requiring explicitly aligned memory.
+  const std::string ExplicitAlign[7] = {"MOVDQA",  "MOVAPS",  "MOVAPD",
+                                        "MOVNTPS", "MOVNTPD", "MOVNTDQ",
----------------
Can these be StringRef instead of std::string?


================
Comment at: utils/TableGen/X86FoldTablesEmitter.cpp:118
+  // For manually mapping instructions that do not match by their encoding.
+  const std::vector<ManualMapEntry> ManualMapSet = {
+      { "ADD16ri_DB",       "ADD16mi",         NO_UNFOLD  },
----------------
Why does this need to be vector? Why not a regular array or std::array?


================
Comment at: utils/TableGen/X86FoldTablesEmitter.cpp:215
+  bool isExplicitAlign(const CodeGenInstruction *Inst) {
+    for (const std::string InstStr : ExplicitAlign) {
+      if (Inst->TheDef->getName().find(InstStr) != std::string::npos)
----------------
This is a string copy. Did you omit an '&'?


================
Comment at: utils/TableGen/X86FoldTablesEmitter.cpp:223
+  bool isExplicitUnalign(const CodeGenInstruction *Inst) {
+    for (const std::string InstStr : ExplicitUnalign) {
+      if (Inst->TheDef->getName().find(InstStr) != std::string::npos)
----------------
string copy


================
Comment at: utils/TableGen/X86FoldTablesEmitter.cpp:234
+      return 0;
+    else if (&Table == &Table0)
+      return 0;
----------------
Don't use 'else' after 'return' per coding standards.


================
Comment at: utils/TableGen/X86FoldTablesEmitter.cpp:372
+
+  if (FormStr == "MRM0r" || FormStr == "MRM1r" || FormStr == "MRM2r" ||
+      FormStr == "MRM3r" || FormStr == "MRM4r" || FormStr == "MRM5r" ||
----------------
Can we extract the X86Local namespace from X86RecognizableInstr.cpp and do this by encodings rather than by string match names?


================
Comment at: utils/TableGen/X86FoldTablesEmitter.cpp:397
+    return true;
+  return false;
+}
----------------
RKSimon wrote:
> ```
> return (Op->getName().find("_NOREX") != std::string::npos);
> ```
Isn't Op->getName() returning a StringRef? Shouldn't that be String::npos?


================
Comment at: utils/TableGen/X86FoldTablesEmitter.cpp:654
+    // Memory -> Register
+    if (FormStr == "MRM0m")
+      return Records.getDef("MRM0r");
----------------
Again I'd like to see if we can use enum encodings from X86Local here.


================
Comment at: utils/TableGen/X86FoldTablesEmitter.cpp:691
+  // Holds all register instructions - divided according to opcode.
+  std::map<uint16_t, std::vector<const CodeGenInstruction *>> RegInsts;
+
----------------
Why is this uint16_t? Isn't opcode only 8-bits?


================
Comment at: utils/TableGen/X86FoldTablesEmitter.cpp:720
+    else if (hasRegisterFormat(Rec)) {
+      uint64_t Opc = getValueFromBitsInit(Rec->getValueAsBitsInit("Opcode"));
+      RegInsts[Opc].push_back(Inst);
----------------
You use uint64_t for Opc here, but uint16_t on line 729. I think really it should be uint8_t for both right?


https://reviews.llvm.org/D32684