[PATCH] D42251: [globalisel][legalizer] Adapt LegalizerInfo to support inter-type dependencies and other things.

[Justin Bogner via Phabricator via llvm-commits]

bogner added a comment.

As far as the diff goes it does look like we end up being much more verbose for simple cases (like, IMPLICIT_DEF), but OTOH the definitions we're adding are doing quite a bit more, so that's probably fine. I think the approach is reasonable.



================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:173-174
+
+  inline LegalizeRuleSet &actionIf(LegalizeAction Action,
+                                   LegalityPredicate Predicate) {
+    add({Predicate, Action});
----------------
inline is redundant here - since we're defining this within the class definition it's implicitly inline.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:194-195
+  inline LegalizeRuleSet &
+  actionForCartesianProd(LegalizeAction Action,
+                         std::initializer_list<LLT> Types) {
+    return actionIf(Action, LegalityPredicates::all(
----------------
If we've already spelled out all of "cartesian" are we really saving anything by abbreviating product to "prod"?


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:196-198
+    return actionIf(Action, LegalityPredicates::all(
+                                LegalityPredicates::typeInSet(0, Types),
+                                LegalityPredicates::typeInSet(1, Types)));
----------------
Probably worth simplifying with a using namespace:

    using namespace LegalityPredicates;
    return actionIf(Action, all(typeInSet(0, Types), typeInSet(1, Types)));


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:246-254
+#if 0
+  LegalizeRuleSet &lowerFor(std::initializer_list<LLT> Types) {
+    return lowerIf(LegalityPredicates::typeInSet(0, Types));
+  }
+
+  LegalizeRuleSet &lowerFor(std::initializer_list<std::pair<LLT, LLT>> Types) {
+    return lowerIf(LegalityPredicates::typePairInSet(0, 1, Types));
----------------
Remove or finish implementing?


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:371-374
+  LegalizeRuleSet &fallback() {
+    add({always, LegalizeAction::UseLegacyRules});
+    return *this;
+  }
----------------
I'm not sure we want this here - this would allow someone to write some new style rules for an op but then fallback as well, which is just confusing. I think we should enforce either new style rules or fallback, and not allow weird combinations of both.


================
Comment at: lib/Target/AArch64/AArch64LegalizerInfo.cpp:53
+      .widenScalarToPow2(0, 8)
+      .unsupported();
 
----------------
Would it make more sense to have the end of the list be implicitly unsupported? I think currently it would go to fallback instead, but as I pointed out earlier I don't think that's the right thing to do.


https://reviews.llvm.org/D42251