[PATCH] D31711: [GlobalISel] LegalizerInfo: Enable legalization of vector types with non-power-of-2 number of elements

[Volkan Keles via llvm-commits]

Hi,

I updated the patch, but Phabricator doesn’t send email notifications. Can you take another look at it?

Thanks,
Volkan

> On Apr 6, 2017, at 7:53 PM, Ahmed Bougacha via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> ab accepted this revision.
> ab added a comment.
> This revision is now accepted and ready to land.
> 
> LGTM after removing the whole check and adding a good explanation in the commit message.
> 
> Can you add a simple vector test in test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll?  It already has a scalar test (that shows the change is OK if it continues reporting a fallback)
> 
> 
> 
> ================
> 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))
> ----------------
> 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.
> 
> 
> https://reviews.llvm.org/D31711
> 
> 
>