[PATCH] D65440: GlobalISel: Add widenScalar for G_UNMERGE_VALUES sources

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1084-1098
+  if (TypeIdx == 1) {
+    Register Cast = MIRBuilder.buildAnyExt(WideTy, SrcReg).getReg(0);
+    MIRBuilder.buildTrunc(MI.getOperand(0), Cast);
+
+    const unsigned DstSize = DstTy.getSizeInBits();
+    unsigned Offset = DstSize;
+    for (unsigned I = 1; I != NumDst; ++I, Offset += DstSize) {
----------------
arsenm wrote:
> dsanders wrote:
> > This feels more like lower than widenScalar as it replaces the G_UNMERGE_VALUES with G_LSHR+G_TRUNC. I would have expected widenScalar to change something like:
> >     %1, %2 = G_UNMERGE_VALUES %0
> > into:
> >     %5 = G_ANYEXT %0
> >     %1, %2, %3, %4 = G_UNMERGE_VALUES %5
> > where %3 and %4 are unused. The main limitation with that is that sizeof(%5) would have to be a multiple of sizeof(%1). If that's not true then it would have to lower instead as G_UNMERGE_VALUES wouldn't be able to handle it.
> > 
> > What was the reason for emitting G_LSHR+G_TRUNC?
> I’m not quite sure where to handle the various steps for merge/unmerge. D65447 is essentially the same, except it bitcasts from a vector which seems more like a lower action. Lower however does not have a specified type for it. I thought using the type hint for these primitives is more relevant than the operation itself.
> 
> I’m also having trouble deciding whether to handle the source or destination type for some of these. The fact that widen/narrow are distinct actions is something of a complication. Sometimes a wider type needs to be introduced when narrowing, and it’s not clear to me how it should be handled. This is sometimes avoidable by doing something resembling the opposite on the other type index.
> 
> I’m also not sure in cases where the types do not evenly divide whether a single legalize step should attempt to deal with it, or if the target legalization rules should have the burden of specifying actions to intermediate types which would be simpler to handle
> I’m not quite sure where to handle the various steps for merge/unmerge. D65447 is essentially the same, except it bitcasts from a vector which seems more like a lower action. Lower however does not have a specified type for it. I thought using the type hint for these primitives is more relevant than the operation itself.

I agree it's rather fuzzy. My understanding is that widenScalar/narrowScalar expect to emit the same core operations but on intermediates of the given type plus some glue MIR to make it all fit. Similarly for moreElements/fewerElements. Lower/Libcall (and Bitcast if we were to add one) expect to change the operations but stick to the same type. Custom doesn't really have any expectations and is free to do whatever.

FWIW, I'd like to push the legalizer more towards only having two real actions: Legal and ChangeIt. Where ChangeIt is given a function that makes the desired change (those functions could be from the widenScalar/lower/etc. family). Everything beyond that like widenScalar/lower/etc. is just sugar. The main reason I want to move in this direction is because it eliminates the Custom action. At the moment if you use Custom then one function has to be capable of every change you might want to make even if different cases are completely different.

> I’m also having trouble deciding whether to handle the source or destination type for some of these. The fact that widen/narrow are distinct actions is something of a complication. Sometimes a wider type needs to be introduced when narrowing, and it’s not clear to me how it should be handled. This is sometimes avoidable by doing something resembling the opposite on the other type index.

Could you show me an example of needing to widen a type as part of narrowing?

> I’m also not sure in cases where the types do not evenly divide whether a single legalize step should attempt to deal with it, or if the target legalization rules should have the burden of specifying actions to intermediate types which would be simpler to handle

You mean for things like widening/narrowing the sources of a G_MERGE_VALUES or the results of a G_UNMERGE_VALUES? Changing the other side of these two is fairly simple (you just add undefs or unused results and G_ANYEXT/G_TRUNC the one that has a remainder) but changing them from this side is tricky.

I think the intent is that a single legalize step is supposed to handle it but it's certainly possible that we haven't given that enough thought.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65440/new/

https://reviews.llvm.org/D65440