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

Wed Jul 19 03:04:37 PDT 2017

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

LGTM with minor 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
----------------
rovka wrote:
> Unrelated change, please commit separately.
This is marked as done, but the change is still in the patch...

================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelector.h:65
 enum {
+  /// Begin a try-block to attempt a match but and jump to OnFail if it is
+  /// unsuccessful.
----------------
but and

================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:94
+          return false;
+        break;
       }
----------------
This break looks redundant.

================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:123
+          return false;
+        break;
+      }
----------------
Redundant.

================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:140
+          return false;
+        break;
+      }
----------------
Redundant.

================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:157
+          return false;
+        break;
+      }
----------------
Redundant.

================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:177
+          return false;
+        break;
+      }
----------------
Redundant.

================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:193
+          return false;
+        break;
+      }
----------------
Redundant.

================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:209
+          return false;
+        break;
+      }
----------------
Redundant.

================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:217
+      int64_t Value = MatchTable[CurrentIdx++];
       DEBUG(dbgs() << "GIM_CheckIntrinsicID(MIs[" << InsnID << "]->getOperand(" << OpIdx
                    << "), Value=" << Value << ")\n");
----------------
This one doesn't print the CurrentIdx when debugging.

================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:234
+          return false;
+        break;
+      }
----------------
Redundant.

================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:246
+          return false;
+        break;
+      }
----------------
Redundant.

================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:237
     }
+    case GIM_Reject:
+      DEBUG(dbgs() << CurrentIdx << ": GIM_Reject");
----------------
dsanders wrote:
> rovka wrote:
> > 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?
> Good point. I think I missed this one because at the moment it only appears as the last opcode of a table to prevent executeMatchTable() from falling off the end.
> 
> I'm going to need it inside GIM_Try blocks once I start nesting them so I'll correct this one to use handleReject() too.
> 
> > 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)
> 
> I'd prefer not to special case the last block since this will get quite confusing once GIM_Try blocks start nesting. I think it's an optimization we should leave until late in development if we do it.
Ok.

================
Comment at: test/TableGen/GlobalISelEmitter.td:213
 // CHECK-NEXT:    // (add:i32 GPR32:i32:$src1, GPR32:i32:$src2) => (ADD:i32 GPR32:i32:$src1, GPR32:i32:$src2)
-// CHECK-NEXT:    GIR_MutateOpcode, /*InsnID*/0, /*RecycleInsnID*/ 0, /*Opcode*/MyTarget::ADD,
+// CHECK-NEXT:    GIR_MutateOpcode, /*InsnID*/0, /*RecycleInsnID*/0, /*Opcode*/MyTarget::ADD,
 // CHECK-NEXT:    GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
----------------
This is also an unrelated whitespace change (I guess it's not as bad as the previous one though, since it only shows up in a couple of places).

================
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 {
----------------
dsanders wrote:
> rovka wrote:
> > This comment sounds weird, can you rephrase?
> The last word was supposed to be 'table' but it's still not particularly clear after that either.
> 
> The main thing I'm trying to convey is that there's no inheritance hierarchy (the MatchTableRecord class supports all the quirks of each kind of record such as adding comment characters and indenting) so that the table can be a simple vector. How about this:
>   This class represents any and all output that may be required to emit the MatchTable. Instances  are most often configured to represent an opcode or value that will be emitted to the table with some formatting but it can also represent commas, comments, and other formatting instructions.
That sounds much better.

================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:255
+    /// record.
+    MTRF_Outdent = 0x40,
+  };
----------------
dsanders wrote:
> rovka wrote:
> > 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).
> Most of them do it on an ad-hoc basis when they need it and hard-code the whitespace if they don't need anything special. The closest example is SelectionDAG which indents its table in a similar way so that it's easy for a human to see the structure of the decisions being made by the state machine.
> 
> Unfortunately, the clang-format approach won't help in this case because it's not aware of the meaning of the data. It's not aware that it ought to keep opcodes and arguments on the same line or that GIM_Try/GIM_Reject should affect indentation to help a human follow the decisions the state machine will make. It would be neat if it was possible to teach clang-format about special cases like this table but that will be more complicated.
Ok, I don't think it makes sense to burden clang-format with this kind of knowledge.

================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:281
+            const MatchTable &Table) const;
+  unsigned size() const { return NumElements; }
+};
----------------
You should make NumElements private and use this instead of accessing it directly (e.g. in MatchTable::push_back).

https://reviews.llvm.org/D35117