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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.
Herald added a subscriber: hintonda.

I generally like the direction of the API.  Again, not really a qualified reviewer, but I added a bunch of small comments on readability of the API choices from a non-expert.  Making this stuff more discoverable would be a major plus and this feels like a potential major step in that direction.



================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:150
+namespace LegalityPredicates {
+LegalityPredicate all(LegalityPredicate P0, LegalityPredicate P1);
+LegalityPredicate typeInSet(unsigned TypeIdx,
----------------
Might call this "and" and have an "or" as well.


================
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
----------------
"more" is vague.  "roundUp"?


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:221
+  inline LegalizeRuleSet &
+  actionForCartesianProd(LegalizeAction Action,
+                         std::initializer_list<LLT> Types) {
----------------
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.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:241
+  }
+  LegalizeRuleSet &legalFor(std::initializer_list<LLT> Types) {
+    return actionFor(LegalizeAction::Legal, Types);
----------------
I suspect a legalIf(Type) and actionIf(Action, Type) override might save some typing.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:256
+  LegalizeRuleSet &libcallIf(LegalityPredicate Predicate) {
+    return actionIf(LegalizeAction::Libcall, Predicate);
+  }
----------------
Honestly, having the cover functions here just adds complexity.  Might be better to spell out the rules in the actionIf form explicitly.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:348
+        },
+        LegalizeMutations::pick(TypeIdx, Ty));
+  }
----------------
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.


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


https://reviews.llvm.org/D42251