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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders 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,
----------------
rovka wrote:
> dsanders wrote:
> > ab wrote:
> > > Are all three opcodes necessary?  It seems like we only need one, that defines an ID from an operand.
> > I think there's a potential argument for merging GIM_RecordInsn and GIM_PushInsnOpDef. They're currently separate because I wasn't completely convinced that all visited instructions needed to be recorded. I'm leaning towards all of them being recorded but I decided to come back to that once the compile-time regression was resolved.
> > 
> > GIM_PopInsn needs to be kept separate for the current DAG walking code. Without it we wouldn't be able to walk something like:
> >   (G_MUL (G_ADD a, b), (G_ADD c, d))
> > since once you descend into either G_ADD you wouldn't be able to visit the other. That said, I agree that tweaking GIM_PushInsnOpDef so that it takes a source instruction ID would achieve the same effect.
> > 
> > Are you ok with me working on this change as an optimisation after the compile-time regression fixes are committed? Currently all my work is bottlenecked on that.
> That looks like a good test case to add :)
> 
> I'm still not convinced that you need a stack though, can't you just record all instructions (as you said you are inclined to do anyway) and express everything in terms of InsnIDs?
I've merged the three opcodes into a single one and added a test like the one above (I used G_SUB on all three nodes to avoid commutativity).

There's a small optimization that could be applied to the table as a result of this but it's a job for another patch. If one rule causes us to record the instruction that def's MIs[1]->getOperand(2), then no subsequent rule needs to do it again.


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelector.h:149
+      const TargetRegisterInfo &TRI, const RegisterBankInfo &RBI,
+      const PredicateBitset &AvailableFeatures) const;
+
----------------
rovka wrote:
> Maybe put the Renderers, TypeObjects etc into a struct, so we don't have to change this signature every time we want to add or modify a state.
> 
> If not, at least consider using ArrayRef's for them.
I've made the following changes to this:
* Removed MIStack as part of another change.
* Combined MIs and Renderers into a mutable State object
* Combined TypeObjects, FeatureBitsets, ComplexPredicates into a constant MatcherInfo object.

AvailableFeatures is still separate because I don't think it quite fits into either group.


================
Comment at: test/TableGen/GlobalISelEmitter.td:71
+// CHECK: RecordedMIVector MIs;
+// CHECK: RecordedMIVector MIStack;
 
----------------
ab wrote:
> Add CHECK lines for ComplexPredicates, TypeObjects, Renderers?  It's not crucial for testing, but it's helpful for reading.
I've added checks to the start of the file for them.


================
Comment at: test/TableGen/GlobalISelEmitter.td:126
+// CHECK-NEXT:    GIM_CheckType, /*MI*/0, /*Op*/1, /*Type*/GILLT_s32,
+// CHECK-NEXT:    GIM_CheckRegBankForClass, /*MI*/0, /*Op*/1, /*RC*/MyTarget::GPR32RegClass
+// CHECK-NEXT:    // MIs[0] src2
----------------
rovka wrote:
> Nitpick: "RegClassID, " (just so it matches the others).
Well spotted.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:245
 
-  std::string defineInsnVar(raw_ostream &OS, const InstructionMatcher &Matcher,
-                            StringRef Value);
-  StringRef getInsnVarName(const InstructionMatcher &InsnMatcher) const;
+  unsigned implicitlyDefineInsnVar(raw_ostream &OS,
+                                   const InstructionMatcher &Matcher);
----------------
rovka wrote:
> Please add a comment here explaining how this should be used and why it's different from defineInsnVar.
It was supposed to be different but the implementations in this patch ended up the same. I must have forgotten to make the change I intended.

I've corrected the implementations and added a comment.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:254
 
-void emit(raw_ostream &OS, SubtargetFeatureInfoMap SubtargetFeatures);
+void emit(raw_ostream &OS);
 
----------------
rovka wrote:
> This doesn't seem to be caused by this patch, but the indentation here and below looks funny.
I'm not sure what happened here but I've fixed it in this patch.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1922
+            [](const Record *A, const Record *B) {
+              if (A->getName() < B->getName())
+                return true;
----------------
rovka wrote:
> Um... return A->getName() < B->getName() ?
It's a lexicographic comparison rather than a pointer comparison. The StringRef returned by Record::getName() provides relational operators that compare the strings (similar to std::string)


https://reviews.llvm.org/D33758