[PATCH] D142084: [RFC][X86][MemFold] Upgrade the mechanism of auto-generated Memory Folding Table

[Kan Shengchen via Phabricator via llvm-commits]

skan accepted this revision.
skan added a comment.
This revision is now accepted and ready to land.

LGTM in general



================
Comment at: llvm/utils/TableGen/X86FoldTablesEmitter.cpp:21
+#include "llvm/Support/X86FoldTablesUtils.h"
+#include "llvm/TableGen/Error.h"
 #include "llvm/TableGen/Record.h"
----------------
What's header file `Error.h` used for?


================
Comment at: llvm/utils/TableGen/X86FoldTablesEmitter.cpp:90
+      OS << "{X86::" << RegInst->TheDef->getName() << ", ";
+      //OS.PadToColumn(40);
+      OS  << "X86::" << MemInst->TheDef->getName() << ", ";
----------------
Drop the comment?


================
Comment at: llvm/utils/TableGen/X86FoldTablesEmitter.cpp:113
 
     bool operator<(const X86FoldTableEntry &RHS) const {
       bool LHSpseudo = RegInst->TheDef->getValueAsBit("isPseudo");
----------------
Is this operator never used?


================
Comment at: llvm/utils/TableGen/X86FoldTablesEmitter.cpp:130
+      bool RHSpseudo = RHS->TheDef->getValueAsBit("isPseudo");
+      if (LHSpseudo != RHSpseudo)
+        return LHSpseudo;
----------------
Why do we need to compare `isPseudo`?


================
Comment at: llvm/utils/TableGen/X86FoldTablesEmitter.cpp:435
+        &Target.getInstruction(Records.getDef(
+            RegRec->getName().substr(0, RegRec->getName().size() - 2)));
+    Result.CannotUnfold = BaseRegInst->isMoveReg ? true : Result.CannotUnfold;
----------------
Do we need to assert the result of `getDef` is not NULL?


================
Comment at: llvm/utils/TableGen/X86FoldTablesEmitter.cpp:442
+    Result.CannotUnfold = BaseRegInst->isMoveReg ? true : Result.CannotUnfold;
+  } else if (RegInstr->isMoveReg && Result.IsStore)
     Result.CannotUnfold = true;
----------------
Why do we need `Result.IsStore` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142084