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

[Justin Bogner via Phabricator via llvm-commits]

bogner added inline comments.


================
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
----------------
dsanders wrote:
> 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).
FWIW, as vague as "more" is here, I actually do find it clearer than "increase". Increase sounds like it could mean some kind of elementwise operation, whereas more says "we want more elements". I think this can be left as is.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:241
+  }
+  LegalizeRuleSet &legalFor(std::initializer_list<LLT> Types) {
+    return actionFor(LegalizeAction::Legal, Types);
----------------
dsanders wrote:
> 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.
If you take an ArrayRef instead of an initializer list then ArrayRef's singleton constructor would mean that we could write .legalFor(s8) instead of .legalFor({s8}) and such. I'm not sure that's worth it though, given the potential for confusion that constructor opens up.


https://reviews.llvm.org/D42251