[PATCH] D153756: [TableGen][GlobalISel] Add Generic MatchTableExecutor Emitter

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:302
 
-class GlobalISelEmitter {
+class GlobalISelEmitter : public GlobalISelMatchTableExecutorEmitter {
 public:
----------------
Make final?


================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:314
+  const CodeGenTarget &getTarget() const override { return Target; }
+  std::string getClassName() const override {
+    return Target.getName().str() + "InstructionSelector";
----------------
Probably should make this return a StringRef and make clients not allocate temporary strings on every debug print 


================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:2271
+        "&CoverageInfo) const {\n"
+     << "  MachineFunction &MF = *I.getParent()->getParent();\n"
+     << "  MachineRegisterInfo &MRI = MF.getRegInfo();\n"
----------------
MF should probably be available as a class member, set in setupMF. Long term I don't think instructions should point to their parents


================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:2272
+     << "  MachineFunction &MF = *I.getParent()->getParent();\n"
+     << "  MachineRegisterInfo &MRI = MF.getRegInfo();\n"
+     << "  const PredicateBitset AvailableFeatures = "
----------------
setupMF already does this but for some reason is just repeated by every target, should just move that to the base class


================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:2302
+      [&](Record *R) {
+        return R->getValueAsString("GISelPredicateCode").str();
+      },
----------------
any way to avoid duplicating all the strings?


================
Comment at: llvm/utils/TableGen/GlobalISelMatchTableExecutorEmitter.cpp:62
+      return false;
+    for (auto Pair : zip(A, B)) {
+      if (std::get<0>(Pair)->getName() < std::get<1>(Pair)->getName())
----------------
C++17 tuple syntax?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153756