[PATCH] D29709: [globalisel] Separate the SelectionDAG importer from the emitter. NFC

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:83-84
   /// Otherwise, return a reason why this pattern was skipped for emission.
-  Optional<SkipReason> runOnPattern(const PatternToMatch &P,
-                                    raw_ostream &OS);
+  Optional<SkipReason> runOnPattern(std::vector<RuleMatcher> &Rules,
+                                    const PatternToMatch &P, raw_ostream &OS);
 };
----------------
ab wrote:
> My original thought was to replace, when we get to this point, the Optional<SkipReason> with an Expected<something>.  I gave that a shot in D29743, what do you think?
I'm in two minds about this at the moment. I originally started this writing this patch as a transformation to Expected<RuleMatcher> but I went with the std::vector<RuleMatcher> in the end due to a need to emit multiple rules for a single pattern. I was thinking about commutativity at the time and was planning to expand, for example, (G_XOR $rs, -1) as two separate rules but I tried to implement commutativity yesterday and it's looking like it will be easier to handle that in the InstructionMatcher. However, I think the multiple-rules requirement may also arise for other cases (e.g. ComplexPattern) so I'm not entirely convinced I don't need the vector anymore.

I suppose it's best to go with Expected<RuleMatcher> until we have a pressing need to expand patterns into multiple rules at which point we can re-assess how we do this.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:564
+  for (const auto &Rule : Rules) {
+    // We're done with this pattern!  Emit the processed result.
+    Rule.emit(OS);
----------------
kristof.beyls wrote:
> This comment has become incorrect in the new version of the code?
> Probably best to just remove?
I wouldn't go as far as saying it's incorrect but I agree it's not serving any real purpose anymore. I'll remove it if we go ahead with this patch rather than D29743


https://reviews.llvm.org/D29709