[PATCH] D35117: [globalisel][tablegen] Add control-flow to the MatchTable.

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

I think the commit message should elaborate a bit on why we want a single table.
Other nits:



================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelector.h:118
+  /// Check the operand is a specific literal integer (i.e. MO.isImm() or
+  /// MO.isCImm() is true).
   /// - InsnID - Instruction ID
----------------
Unrelated change, please commit separately.


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:33
+  enum { RejectAndGiveUp, RejectAndResume };
+  auto handleReject = [&]() {
+    DEBUG(dbgs() << CurrentIdx << ": Rejected\n");
----------------
I think omitting the return type for a lambda with more than one return statement is C++14, you might run into trouble with some of the buildbots.


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:237
     }
+    case GIM_Reject:
+      DEBUG(dbgs() << CurrentIdx << ": GIM_Reject");
----------------
Both the comment on this and the summary say that this either exists the current try block or completely fails to match if there is no try block. That's not what this does, this just completely fails. Your lambda seems to take care of the logic for exiting the current block or bailing out, so it seems like this state is really unnecessary - it would be enough to just not generate a Try block at all for the last instruction that you're matching (so you wouldn't actually need to generate a Try/Reject pair for any of the current tests). Am I missing something?


================
Comment at: test/TableGen/GlobalISelEmitter.td:100
+// CHECK-NEXT:    GIM_Try, /* On fail goto */ /* Label 0 */ [[LABEL:[0-9]+]],
+// CHECK-NEXT:    GIM_CheckNumOperands, /* MI */ 0, /* Expected */ 4,
+// CHECK-NEXT:    GIM_CheckOpcode, /* MI */ 0, TargetOpcode::G_SELECT,
----------------
The spaces in the comments are really unrelated, can you get rid of them for this patch?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:234
+/// A record to be stored in a MatchTable.
+/// This class supports all kinds of data that needs to be stored in the data.
+struct MatchTableRecord {
----------------
This comment sounds weird, can you rephrase?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:255
+    /// record.
+    MTRF_Outdent = 0x40,
+  };
----------------
How do the other TableGen backends handle indentation? Personally I preferred your previous patch which just ran clang-format on the generated files, since that kept the backends simple. But if other people don't like that approach, we should at least try to share some of this stuff between backends (if it wouldn't be too convoluted).


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:454
   /// A map of instruction matchers to the local variables created by
-  /// emitCxxCaptureStmts().
+  /// emitCaptureOpcodes().
   std::map<const InstructionMatcher *, unsigned> InsnVariableIDs;
----------------
Unrelated change, please commit separately.


https://reviews.llvm.org/D35117