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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

This needs a testcase.



================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:315
 
+  Optional<std::string> Predicate;
   std::vector<std::unique_ptr<InstructionMatcher>> Matchers;
----------------
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


================
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:
> 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.


https://reviews.llvm.org/D29478