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

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

Not for this patch, but I feel like some of the names and comments (e.g. emitCxxCaptureStmts) are starting to drift a bit from the implementation. Maybe at the end of this patch stack you could look into brushing them up?



================
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,
----------------
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?


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelector.h:149
+      const TargetRegisterInfo &TRI, const RegisterBankInfo &RBI,
+      const PredicateBitset &AvailableFeatures) const;
+
----------------
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.


================
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
----------------
Nitpick: "RegClassID, " (just so it matches the others).


================
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);
----------------
Please add a comment here explaining how this should be used and why it's different from defineInsnVar.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:254
 
-void emit(raw_ostream &OS, SubtargetFeatureInfoMap SubtargetFeatures);
+void emit(raw_ostream &OS);
 
----------------
This doesn't seem to be caused by this patch, but the indentation here and below looks funny.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1922
+            [](const Record *A, const Record *B) {
+              if (A->getName() < B->getName())
+                return true;
----------------
Um... return A->getName() < B->getName() ?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:2038
+      continue;
+    OS << "  GIFBS";
+    for (const auto &Feature : FeatureBitset)
----------------
I would add a static helper for generating these names, so it's easier to keep in sync with the use in RuleMatcher::emit.


https://reviews.llvm.org/D33758