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

[Ahmed Bougacha via Phabricator via llvm-commits]

ab 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);
 };
----------------
dsanders wrote:
> 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.
IIRC things like commutativity are handled as extra PatternToMatch entries, somewhere when we do the initial CodeGenDAGPatterns parse.  If that's true, it does mean we'll get mostly-identical rules.  So you're right, doing something in InstructionMatcher is probably the smart move.

But either way, can't we do Expected<std::vector<RuleMatcher>> ?  It's a tad more expensive, but it seems like we could still move all the RuleMatchers.


https://reviews.llvm.org/D29709