[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:
> 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.
CodeGenDAGPatterns::AddPatternToMatch() is rejecting the commutative cases on the grounds that they can never match. This is because DAGCombiner::visitADD() canonicalizes ISD::ADD nodes such that the immediate is always on the right hand side.


https://reviews.llvm.org/D29709