[PATCH] D28942: [globalisel] Re-factor ISel matchers into a hierarchy. NFC

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

> One quick question: how do you envision nested patterns/instructions? Would one of the parent's OperandMatchers have some sort of "instruction" predicate?

That's right. I'm expecting to have an OperandPredicateMatcher subclass that follows the DefUse chain to the def. It will then use an InstructionMatcher to match the nested instruction.

I'm still thinking through a couple details of the generated code that this case will need but it doesn't affect the representation of within tablegen. One issue is that it's possible for side-effects/memory/etc. between the matched instructions to prevent the match. I don't want to implement that by walking the intervening instructions since that's likely to be slow and might involve traversing the CFG in the long-run. The other one is what to do if the nested instruction has multiple uses since there's no single good answer for all situations. It depends on the target and whether the priority is performance/size/energy.



================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:253-256
+  template <class Kind, class... Args>
+  Kind &addPredicate(Args... args) {
+    Predicates.emplace_back(new Kind(std::forward<Args...>(args)...));
+    return *static_cast<Kind *>(Predicates.back().get());
----------------
kristof.beyls wrote:
> It stood out to me that the method with the same name on class OperandMatcher has the following, slightly different implementation:
>   template <class Kind, class... Args>
>   Kind &addPredicate(Args... args) {
>     Predicates.emplace_back(make_unique<Kind>(std::forward<Args...>(args)...));
>     return *static_cast<Kind *>(Predicates.back().get());
>   }
> 
> Any reason it has to be different?
> Any reasonable way possible to not have to repeat this implementation, but be able to share a single implementation between both classes?
> Any reason it has to be different?

Well spotted. They're not supposed to be different.

> Any reasonable way possible to not have to repeat this implementation, but be able to share a single implementation between both classes?

I think I can have InstructionMatcher and OperandMatcher subclass a template that defines the predicates and predicate-related methods. I'll take a look


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:269-280
+    for (auto &Predicate : Predicates) {
+      OS << Separator << "(";
+      Predicate->emitCxxPredicateExpr(OS, InsnVarName);
+      OS << ")";
+      Separator = " && ";
+    }
+    for (auto &Operand : Operands) {
----------------
kristof.beyls wrote:
> To make the generated code more efficient, we'd probably want to play heuristics here so that the most discriminating predicate gets checked first?
> Although I guess that's an optimization that can wait for later, until this design has proven to work well?
I agree. I think we should do that once we're successfully importing most of the SelectionDAG rules since there's likely to be a fair bit of iteration as we gain support for the more complicated ones.

Another optimization that will be important when we get to that will be hoisting common predicates out of the rules to reduce the amount that need to be checked. The main one will be the opcode check since implementing that as a switch that picks between different rule sets will dramatically cut down on the number of rules to check.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:294
 
-class MatcherEmitter {
+class MatcherRule {
   const PatternToMatch &P;
----------------
kristof.beyls wrote:
> I think this needs a comment to explain what a MatcherRule is/does.
> I think the comments on the classes above are really helpful to be able to understand the code and intent quickly.
Ok. I'll also rename it to RuleMatcher to make it consistent with InstructionMatcher/OperandMatcher


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:297-300
+  std::vector<std::unique_ptr<InstructionMatcher>> Matchers;
+
 public:
   std::vector<std::unique_ptr<MatchAction>> Actions;
----------------
kristof.beyls wrote:
> My guess it'd make sense for Matchers and Actions to either both be private or both be public?
> My personal preference would be both private, but I'm not sure if the coding standard has anything to say on this.
I think they should both be private but I wanted to stick to the matchers for this patch.

I'm also not sure we need support for multiple actions, since they aren't as easy to compose as the matchers are. My initial thoughts on the actions were to have a mutate operation (to just change the opcode), a simple replacement operation, an arbitrary C++ operation, and maybe a couple other common special cases.


https://reviews.llvm.org/D28942