[PATCH] D33758: [globalisel][tablegen] Partially fix compile-time regressions by converting matcher to state-machine(s)

[Ahmed Bougacha via Phabricator via llvm-commits]

ab added inline comments.


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelector.h:63-71
+  /// Record the specified instruction
+  /// - NewInsnID - Instruction ID to define
+  GIM_RecordInsn,
+  /// Follow the specified operand to the def and push it onto the stack
+  /// - InsnID - Instruction ID
+  /// - OpIdx - Operand index
+  GIM_PushInsnOpDef,
----------------
Are all three opcodes necessary?  It seems like we only need one, that defines an ID from an operand.


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:30
+    const PredicateBitset &AvailableFeatures) const {
+  const int64_t *Command = MatchTable;
+  while (true) {
----------------
Should this be unsigned?

Also, is int64_t necessary?  Seems like almost everything should fit in 16 bits.  X86 is close to 15k opcodes, but we can complain when we emit something that doesn't fit.  As for CheckInt, we can just encode the 64-bit value across 4 operands.


================
Comment at: test/TableGen/GlobalISelEmitter.td:71
+// CHECK: RecordedMIVector MIs;
+// CHECK: RecordedMIVector MIStack;
 
----------------
Add CHECK lines for ComplexPredicates, TypeObjects, Renderers?  It's not crucial for testing, but it's helpful for reading.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1984-1993
+  std::vector<LLTCodeGen> TypeObjects = {
+      LLT::scalar(8),      LLT::scalar(16),     LLT::scalar(32),
+      LLT::scalar(64),     LLT::scalar(80),     LLT::vector(8, 1),
+      LLT::vector(16, 1),  LLT::vector(32, 1),  LLT::vector(64, 1),
+      LLT::vector(8, 8),   LLT::vector(16, 8),  LLT::vector(32, 8),
+      LLT::vector(64, 8),  LLT::vector(4, 16),  LLT::vector(8, 16),
+      LLT::vector(16, 16), LLT::vector(32, 16), LLT::vector(2, 32),
----------------
Instead of a hardcoded list, MVTToLLT whatever can be converted in MVT::all_valuetypes() ?


https://reviews.llvm.org/D33758