[PATCH] D31711: [GlobalISel] LegalizerInfo: Enable legalization of vector types with non-power-of-2 number of elements
Kristof Beyls via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 6 02:40:44 PDT 2017
kristof.beyls added inline comments.
================
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:
> 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?
https://reviews.llvm.org/D31711
More information about the llvm-commits
mailing list