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

[Ayman Musa via Phabricator via llvm-commits]

aymanmus marked 15 inline comments as done.
aymanmus added inline comments.


================
Comment at: test/CodeGen/X86/stack-folding-fp-avx1.ll:1654
 
-define double @stack_fold_sqrtsd(double %a0) {
-  ;CHECK-LABEL: stack_fold_sqrtsd
----------------
RKSimon wrote:
> Please don't delete tests, especially if they've regressed - show the new codegen and add a todo comment
The test checks that specific instructions are memory folded.
In the removed test cases, the instructions were removed from the memory folding tables so testing that these instructions are NOT folded in this test, is not the best idea.


================
Comment at: utils/TableGen/X86FoldTablesEmitter.cpp:69
+      else
+        result.erase(result.length() - 3, 3);
+
----------------
craig.topper wrote:
> 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.
Thanks for the suggestion.


================
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  },
----------------
craig.topper wrote:
> Why does this need to be vector? Why not a regular array or std::array?
Defining an array as a class member requires stating it's size (ManualMapSet[18]), while std::vector doesn't. I thought this table might be dynamic and changed from time to time, so this way can be more friendly for future tuning.
What do you think?


================
Comment at: utils/TableGen/X86FoldTablesEmitter.cpp:372
+
+  if (FormStr == "MRM0r" || FormStr == "MRM1r" || FormStr == "MRM2r" ||
+      FormStr == "MRM3r" || FormStr == "MRM4r" || FormStr == "MRM5r" ||
----------------
craig.topper wrote:
> Can we extract the X86Local namespace from X86RecognizableInstr.cpp and do this by encodings rather than by string match names?
This is very helpful, thanks for the suggestion.


https://reviews.llvm.org/D32684