[PATCH] D31711: [GlobalISel] LegalizerInfo: Check if the vector element type is power of 2

[Volkan Keles via Phabricator via llvm-commits]

volkan 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))
----------------
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



https://reviews.llvm.org/D31711