[PATCH] D29478: [GlobalISel] Generate selector with predicates; use it for FP binops.

[Ahmed Bougacha via Phabricator via llvm-commits]

ab added a comment.

In https://reviews.llvm.org/D29478#665821, @dsanders wrote:

> This needs a testcase.


Right!  I've been wanting to write tablegen tests forever but never remember.  I guess it just doesn't come naturally in these parts ;)  Done in r294075.



================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:315
 
+  Optional<std::string> Predicate;
   std::vector<std::unique_ptr<InstructionMatcher>> Matchers;
----------------
ab wrote:
> dsanders wrote:
> > dsanders wrote:
> > > kristof.beyls wrote:
> > > > I've gotten the feedback that most developers don't really understand how the auto-generated "magic" works for DAGISel.
> > > > So far, I think the current design is pretty easy to understand in comparison. I've tried being picky in this area before about things like naming to keep the code as-easy-to-understand as possible.
> > > > Maybe the 3 variables here could use a short comment in that respect?
> > > > Maybe something like
> > > > 
> > > > ```
> > > >   /// optional code string that indicates when this rule is legal
> > > >   Optional<std::string> Predicate;
> > > >   /// list of matchers that all need to evaluate true for the current IR node to match pattern P.
> > > >   std::vector<std::unique_ptr<InstructionMatcher>> Matchers;
> > > >   /// re-writing actions that need to be taken when the current IR node matches pattern P and Predicate is true.
> > > >   std::vector<std::unique_ptr<MatchAction>> Actions;
> > > > ```
> > > > 
> > > > Please note that I haven't tried hard to make the comment fully factual correct - I'm hoping Daniel or Ahmed will be able to quickly make those comments factually correct.
> > > I'd prefer it if we did these predicates the same way as for Instructions and Operands (i.e. with PredicateListMatcher<RulePredicateMatcher>). This is because I'm trying to push the SelectionDAG dependency out of the *Matcher classes so we can eventually import pure-GlobalISel rules in addition to the imported SelectionDAG rules. I also expect we'll eventually have GlobalISel-specific predicates that control when rules are applied (e.g. ISel, Pseudo-expansion, etc.)
> > > 
> > > Reaching that point isn't a near term goal though so I don't have any strong objections to importing the whole predicate expression for now and making it more precise later
> > These comments are accurate. I'd add a FIXME saying that Matchers is only allowed to be 1-element long for now until we have a way to match multiple independent instructions.
> Done with a few tweaks in r294077, let me know if you spot anything else!
Yeah, that's what I originally did, but it felt quite boilerplate-y, so I figured we keep it simple in the meantime.  Either works though.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:315-317
+  Optional<std::string> Predicate;
   std::vector<std::unique_ptr<InstructionMatcher>> Matchers;
   std::vector<std::unique_ptr<MatchAction>> Actions;
----------------
dsanders wrote:
> dsanders wrote:
> > kristof.beyls wrote:
> > > I've gotten the feedback that most developers don't really understand how the auto-generated "magic" works for DAGISel.
> > > So far, I think the current design is pretty easy to understand in comparison. I've tried being picky in this area before about things like naming to keep the code as-easy-to-understand as possible.
> > > Maybe the 3 variables here could use a short comment in that respect?
> > > Maybe something like
> > > 
> > > ```
> > >   /// optional code string that indicates when this rule is legal
> > >   Optional<std::string> Predicate;
> > >   /// list of matchers that all need to evaluate true for the current IR node to match pattern P.
> > >   std::vector<std::unique_ptr<InstructionMatcher>> Matchers;
> > >   /// re-writing actions that need to be taken when the current IR node matches pattern P and Predicate is true.
> > >   std::vector<std::unique_ptr<MatchAction>> Actions;
> > > ```
> > > 
> > > Please note that I haven't tried hard to make the comment fully factual correct - I'm hoping Daniel or Ahmed will be able to quickly make those comments factually correct.
> > I'd prefer it if we did these predicates the same way as for Instructions and Operands (i.e. with PredicateListMatcher<RulePredicateMatcher>). This is because I'm trying to push the SelectionDAG dependency out of the *Matcher classes so we can eventually import pure-GlobalISel rules in addition to the imported SelectionDAG rules. I also expect we'll eventually have GlobalISel-specific predicates that control when rules are applied (e.g. ISel, Pseudo-expansion, etc.)
> > 
> > Reaching that point isn't a near term goal though so I don't have any strong objections to importing the whole predicate expression for now and making it more precise later
> These comments are accurate. I'd add a FIXME saying that Matchers is only allowed to be 1-element long for now until we have a way to match multiple independent instructions.
Done with a few tweaks in r294077, let me know if you spot anything else!


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:349-369
     OS << "  // Src: " << *P.getSrcPattern() << "\n"
        << "  // Dst: " << *P.getDstPattern() << "\n";
 
+    OS << "  if (";
+
+    // Emit the rule-level predicates, if any.
+    if (Predicate)
----------------
kristof.beyls wrote:
> I think it's slightly easier to read if the assert would go before any of the code-emitting code.
> It makes it slightly easier to read what code gets generated.
Fair enough; done.


https://reviews.llvm.org/D29478