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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders marked 9 inline comments as done.
dsanders added inline comments.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:371-374
+  LegalizeRuleSet &fallback() {
+    add({always, LegalizeAction::UseLegacyRules});
+    return *this;
+  }
----------------
dsanders wrote:
> bogner wrote:
> > dsanders wrote:
> > > 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.
> > Right. I'd rather falling off the end of the ruleset mean unsupported, and I don't really see a reason to allow fallback for an op once you've started writing the rules for that op.
> Ok, I can switch it over to that
Kept fallback() but made falling off the end of the rules mean Unsupported


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:371-374
+  LegalizeRuleSet &fallback() {
+    add({always, LegalizeAction::UseLegacyRules});
+    return *this;
+  }
----------------
dsanders wrote:
> dsanders wrote:
> > bogner wrote:
> > > dsanders wrote:
> > > > 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.
> > > Right. I'd rather falling off the end of the ruleset mean unsupported, and I don't really see a reason to allow fallback for an op once you've started writing the rules for that op.
> > Ok, I can switch it over to that
> Kept fallback() but made falling off the end of the rules mean Unsupported
I've kept fallback() because falling of the end of the ruleset now means unsupported


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:150
+namespace LegalityPredicates {
+LegalityPredicate all(LegalityPredicate P0, LegalityPredicate P1);
+LegalityPredicate typeInSet(unsigned TypeIdx,
----------------
reames wrote:
> Might call this "and" and have an "or" as well.
Unfortunately `and` is a reserved keyword. It's equivalent to `&&` but can be represented in 7-bit ASCII.

I haven't needed an `any` yet, but we can add it when needed.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:166
+LegalizeMutation widenScalarToPow2(unsigned TypeIdx, unsigned Min = 0);
+LegalizeMutation moreElementsToPow2(unsigned TypeIdx, unsigned Min = 0);
+} // end namespace LegalizeMutations
----------------
reames wrote:
> "more" is vague.  "roundUp"?
I've named it after the LegalizeActions::MoreElements action but I agree the end result is weirdly named. I've changed them to `widenScalarToNextPow2` and `moreElementsToNextPow2` to begin with but the latter would make more sense as increaseElementsToNextPow2. If we do that, then we should also change MoreElements to IncreaseElements (and similarly FewerElements to ReduceElements).


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:221
+  inline LegalizeRuleSet &
+  actionForCartesianProd(LegalizeAction Action,
+                         std::initializer_list<LLT> Types) {
----------------
reames wrote:
> You could spell this with a lazily evaluated CartesianProduct class.  If the interface took an arbitrary generator object, it would be powerful with less code duplication.
I'm not sure what you mean here. This function is avoiding computing the cartesian product by testing each type index against the same list.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:241
+  }
+  LegalizeRuleSet &legalFor(std::initializer_list<LLT> Types) {
+    return actionFor(LegalizeAction::Legal, Types);
----------------
reames wrote:
> I suspect a legalIf(Type) and actionIf(Action, Type) override might save some typing.
I'm reluctant to make it take a single type. I'd like to discourage something like:
  .legalIf(s8)
  .legalIf(s16)
  .legalIf(s32)
  .legalIf(s64)
since that pays the memory, loop, and function call overhead four times per instruction being legalized whereas:
  .legalFor({s8, s16, s32, s64})
pays it once.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:256
+  LegalizeRuleSet &libcallIf(LegalityPredicate Predicate) {
+    return actionIf(LegalizeAction::Libcall, Predicate);
+  }
----------------
reames wrote:
> Honestly, having the cover functions here just adds complexity.  Might be better to spell out the rules in the actionIf form explicitly.
This particular one is for consistency with the other *If() functions and isn't being used at the moment. For the cover functions in general: Aside from aesthetics in the target LegalizerInfo implementations, the main reason the cover functions are there is to present the right interface to the right action. Some action values for actionIf are only required to provide an action and predicate function, but others also need to provide a mutation function. I could make it an assertion but I want it to fail at compile-time.

What I originally wanted to do was to declare templates and then name particular instantiations. Something like:
  template<LegalizeAction Action> LegalizeRuleSet &actionIf(LegalityPredicate Predicate) { ... }
  template<LegalizeAction Action> LegalizeRuleSet &actionIfAndMutate(LegalityPredicate Predicate, LegalizeMutation Mutate) { ... }
  using legalIf = actionIf<Legal>;
  using unsupportedIf = actionIf<Unsupported>;
  using widenScalarIf = actionIfAndMutate<WidenScalar>;
  using narrowScalarIf = actionIfAndMutate<WidenScalar>;
but `using` can only create aliases for types.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:348
+        },
+        LegalizeMutations::pick(TypeIdx, Ty));
+  }
----------------
reames wrote:
> You might want to choose a different verb than "pick" here.  Or at least, it isn't obvious from just reading the code what this does.
One of my colleagues has said the same thing in person a couple days ago but I haven't come up with anything better yet. The intent is that it returns a mutation function that always returns the given typeidx and type.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:363
+
+  LegalizeRuleSet &minNumElements(unsigned TypeIdx, const LLT &EltTy, unsigned MinElements) {
+    return moreElementsIf(
----------------
reames wrote:
> min and max feel like the wrong names.  They sound like predicates, not actions.  Maybe increaseNumElements?  Or clampMinNumElements?
I've gone with clampMinNumElements


https://reviews.llvm.org/D42251