[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:
> > > 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.
> Interesting;  I suppose immediates aren't a great example, as we should also (eventually) do the canonicalization.
> 
> What about things like (mul x, (add y, z)) ?  Do we need to handle commutativity for those ourselves?
I think we do need to handle those cases. AddPatternToMatch() isn't rejecting those and both patterns show up separately in AArch64GenDAGISel.inc. 

For example: AArch64 has these entries in MatcherTable:
  // Src: (add:i64 (mul:i64 (sext:i64 GPR32:i32:$Rn), (imm:i64)<<P:Predicate_s64imm_32bit>>:$C), GPR64:i64:$Ra) - Complexity = 18
  // Dst: (SMADDLrrr:i64 GPR32:i32:$Rn, (MOVi32imm:i32 (trunc_imm:i32 (imm:i64):$C)), GPR64:i64:$Ra)

  // Src: (add:i64 GPR64:i64:$Ra, (mul:i64 (sext:i64 GPR32:i32:$Rn), (imm:i64)<<P:Predicate_s64imm_32bit>>:$C)) - Complexity = 18
  // Dst: (SMADDLrrr:i64 GPR32:i32:$Rn, (MOVi32imm:i32 (trunc_imm:i32 (imm:i64):$C)), GPR64:i64:$Ra)


https://reviews.llvm.org/D29709