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

[Kristof Beyls via Phabricator via llvm-commits]

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.


https://reviews.llvm.org/D29478