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

Kristof Beyls via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 3 04:35:42 PST 2017

kristof.beyls added inline comments.

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;
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.

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)
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.


More information about the llvm-commits mailing list