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

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added inline comments.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:210
+
+/// Generates code to check that a predicate on an instruction.
+///
----------------
s/that// ?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:212-214
+/// Typical predicates include:
+/// * The opcode of the instruction is a particular value.
+/// * The nsw/nuw flag is/isn't set.
----------------
Comments like this help to understand the code at least a little bit to someone like me who hasn't looked at tablegen much before. Please keep them :)


================
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());
----------------
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?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:269-280
+    for (auto &Predicate : Predicates) {
+      OS << Separator << "(";
+      Predicate->emitCxxPredicateExpr(OS, InsnVarName);
+      OS << ")";
+      Separator = " && ";
+    }
+    for (auto &Operand : Operands) {
----------------
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?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:294
 
-class MatcherEmitter {
+class MatcherRule {
   const PatternToMatch &P;
----------------
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.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:297-300
+  std::vector<std::unique_ptr<InstructionMatcher>> Matchers;
+
 public:
   std::vector<std::unique_ptr<MatchAction>> Actions;
----------------
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.


https://reviews.llvm.org/D28942