[PATCH] D138545: [VectorCombine] Enable scalarizeBinopOrCmp for scalable vectors

[Matt Devereau via Phabricator via llvm-commits]

MattDevereau added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:1724
+
+    if (isa<VectorType>(I.getType()))
+      MadeChange |= scalarizeBinopOrCmp(I);
----------------
spatel wrote:
> Add a comment here to make the logic difference explicit:
>   // This transform works with scalable and fixed vectors.
> 
> Should there be a TODO comment about allowing more scalable transforms?
I'll add the comment on push.
Regarding the TODO: I think it's ok to put in to signal that there's more scalable optimization work possible here, but to be explicit on why this patch exists, we currently just want to fix this single regression case we've identified and are not actively seeking more scalable optimizations in this area just yet.


================
Comment at: llvm/test/Transforms/VectorCombine/AArch64/scalarize-scalable.ll:4
+
+define <vscale x 4 x float> @scalarize_scalable(float %0, float %1, float %2, float %3) {
+; CHECK-LABEL: @scalarize_scalable(
----------------
spatel wrote:
> This shows a series of folds, but it would be good to have a couple of minimum tests too. Something like this should give us coverage for integer types and compares?
> 
> 
> ```
> define <vscale x 4 x i32> @scalarize_scalable_udiv(i32 %x, i32 %y) {
>   %splatx = insertelement <vscale x 4 x i32> poison, i32 %x, i64 0
>   %splaty = insertelement <vscale x 4 x i32> poison, i32 %y, i64 0
>   %r = udiv <vscale x 4 x i32> %splatx, %splaty
>   ret <vscale x 4 x i32> %r
> }
> 
> define <vscale x 4 x i1> @scalarize_scalable_icmp(i32 %x, i32 %y) {
>   %splatx = insertelement <vscale x 4 x i32> poison, i32 %x, i64 0
>   %splaty = insertelement <vscale x 4 x i32> poison, i32 %y, i64 0
>   %r = icmp sgt <vscale x 4 x i32> %splatx, %splaty
>   ret <vscale x 4 x i1> %r
> }
> 
> ```
Sure, I'm perfectly happy to throw in some more tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138545