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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

In https://reviews.llvm.org/D42251#980841, @bogner wrote:

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


We can probably compress it a bit more. For example:

  getActionDefinitions(G_IMPLICIT_DEF)
      .legalFor({p0, s1, s8, s16, s32, s64})
      .clampScalar(0, s1, s64)
      .widenScalarToPow2(0, 8)
      .unsupported();

down to:

  getActionDefinitions(G_IMPLICIT_DEF)
      .legalFor({p0, s1, s8, s16, s32, s64})
      .changeScalarToNextBiggest(0, {s1, s8, s16, s32, s64});

this particular case is likely to be handled the same way on all targets so we could potentially have:

  getActionDefinitions(G_IMPLICIT_DEF)
      .legalFor({p0, s1})
      .default(/*BiggestType*/ s64);

where `.default()` adds rules for {s8, s16, s32, s64}.



================
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));
----------------
bogner wrote:
> Remove or finish implementing?
Finish implementing. Lower needs a bit more information than I had expected and I need to figure out what it's doing with that information.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:371-374
+  LegalizeRuleSet &fallback() {
+    add({always, LegalizeAction::UseLegacyRules});
+    return *this;
+  }
----------------
bogner wrote:
> 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.
This was intended to be a migration tool but it's not actually in use anymore. I'll remove it.

At the moment, if you fall off the end of the ruleset (which is prevented by the `.unsupported()` calls) then it will fall back on the old implementation. This lets you implement the `.legal*()` part of the rule first and then figure out how to write the legalization rules. I want to push towards deleting the old implementation as soon as possible.


================
Comment at: lib/Target/AArch64/AArch64LegalizerInfo.cpp:53
+      .widenScalarToPow2(0, 8)
+      .unsupported();
 
----------------
bogner wrote:
> 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.
In the long run, yes. Once we've all moved to this then we'll be able to make it the default. Until then, we need a way to fallback on the existing implementation when there are no rules specified or when they are incomplete


https://reviews.llvm.org/D42251