[PATCH] D31711: [GlobalISel] LegalizerInfo: Enable legalization of non-power-of-2 types

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls accepted this revision.
kristof.beyls added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerInfo.cpp:78-81
+  LLT Ty = Aspect.Type;
+  unsigned SizeInBits =
+      Ty.isVector() ? Ty.getElementType().getSizeInBits() : Ty.getSizeInBits();
+  if (!isPowerOf2_64(SizeInBits))
----------------
volkan wrote:
> ab wrote:
> > kristof.beyls wrote:
> > > volkan wrote:
> > > > kristof.beyls wrote:
> > > > > I was confused by the summary on this patch - I thought you wanted to make sure vector types like <3 x s32> definitely wouldn't be legalized and thought the existing code could sometimes legalize them. It then started doing the following train-of-thought:
> > > > > """
> > > > > For vector types, getSizeInBits() returns the "number of elements" times "element size in bits".
> > > > > That result can only be a power of 2 if both "number of elements" and "element size in bits" are both a power of 2.
> > > > > So, the effect of this change would be to allow more vector data types, like <3 x s32> to be legalized."
> > > > > 
> > > > > So, in summary, by the time this gets committed, I think the commit message must be clearer, and e.g. state "Enable legalization of vector types with non-power-of-2 number of elements".
> > > > > 
> > > > > This patch clearly is missing tests.
> > > > > 
> > > > > I was confused by the summary on this patch - I thought you wanted to make sure vector types like <3 x s32> definitely wouldn't be legalized and thought the existing code could sometimes legalize them. It then started doing the following train-of-thought:
> > > > > """
> > > > > For vector types, getSizeInBits() returns the "number of elements" times "element size in bits".
> > > > > That result can only be a power of 2 if both "number of elements" and "element size in bits" are both a power of 2.
> > > > > So, the effect of this change would be to allow more vector data types, like <3 x s32> to be legalized."
> > > > > 
> > > > > So, in summary, by the time this gets committed, I think the commit message must be clearer, and e.g. state "Enable legalization of vector types with non-power-of-2 number of elements".
> > > > > 
> > > > 
> > > > Yes, it seems I mixed the intent and the patch. I'll update the title.
> > > > 
> > > > > This patch clearly is missing tests.
> > > > 
> > > > I didn’t add a test case because there wasn't any legal operations on vector types with non-power-of-2 number of elements, but I just realized I can use G_MERGE_VALUES.
> > > > 
> > > > Thank you,
> > > > Volkan
> > > > 
> > > Thanks for those changes.
> > > 
> > > I'm wondering now if we shouldn't just completely drop the following:
> > > ```
> > >   if (!isPowerOf2_64(SizeInBits))
> > >     return std::make_pair(Unsupported, LLT());
> > > ```
> > > and just let the fall-back mechanism catch the many/all cases where non-power-of-2-sized types aren't supported.
> > > Removing that if statement alltogether makes it a bit easier to also work towards legalizing non-power-of-2 scalar types.
> > > IIRC, Tim introduced this, so I guess he has the best view on the tradeoffs here.
> > > 
> > > @t.p.northover : Do you have any thoughts on this?
> > I agree, we should probably drop the check completely.
> > 
> > I think it was more of a defensive way to avoid even considering non-power-of-2 types elsewhere.  But looking around, it seems we're robust enough to fail legalization properly, so it's fine to do that.
> I think we can’t drop the check right now because LegalizerHelper is not able to handle these cases. If we remove the check, LegalizerInfo is going to try to find a legal type for non-power-of-2 types and the compiler may hit the assertions in LLT::halfScalarSize() or LLT::halfElements() as they don’t expect a non-power-of-2 type.
> 
> Instead, we can just check if a non-power-of-2 type is marked as Legal or Custom and return Unsupported if not. What do you think?
Right - I forgot about the doubling/halving behaviour of LegalizerHelper right now.
TBH, I think the way forward is to get more reviews going on https://reviews.llvm.org/D30529, which makes the other parts of the legalizer handle non-power-of-2 types.
If you need to have support urgently for 3-element vector types, I'd be fine with this going in, but as stated above, I think helping towards making D30529 reach a committable state is probably the most constructive way forward.
I this goes in, I think it would be good to add a FIXME here explaining this is a temporary hack until general non-power-of-2 legalization works.


https://reviews.llvm.org/D31711