[PATCH] D95363: [SVE][LoopVectorize] Add support for scalable vectorization of loops with vector reverse

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2795
+        Value *NumElt =
+            Builder.CreateMul(ConstantInt::get(Builder.getInt32Ty(), -Part),
+                              getRuntimeVF(Builder, Builder.getInt32Ty(), VF));
----------------
Maybe better to use `Builder.getInt32(-Part)` to be consistent with how it's done for the fixed width case?


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-vector-reverse.ll:14
+
+define void @vector_reverse_double(i32 %N, double* nocapture %a, double* nocapture readonly %b) #0{
+; CHECK-LABEL: @vector_reverse_double
----------------
If you use a i64 variable here then it makes the tests simpler as the variable doesn't need sign-extending with `sext`


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-vector-reverse.ll:21
+; CHECK-NEXT: %[[MUL:.*]] = mul i32 %[[VSCALE]], 8
+; CHECK-NEXT: %[[MUL1:.*]] = mul i32 0, %[[MUL]]
+; CHECK-NEXT: %[[GEP1:.*]] = getelementptr inbounds double, double* %[[GEP]], i32 %[[MUL1]]
----------------
Hi @CarolineConcatto something doesn't look right here because the result will be zero.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-vector-reverse.ll:50
+for.body.preheader:                               ; preds = %entry
+  %0 = zext i32 %N to i64
+  br label %for.body
----------------
See comment about `N` - this line can be killed if you have "i64 N", then everywhere that uses %0 can just use %N instead. If you do this then the `for.body.preheader` block can be killed too and the branch above can jump straight to `for.body`


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-vector-reverse.ll:70
+
+define void @vector_reverse_longint(i32 %N, i64* nocapture %a, i64* nocapture readonly %b) #0 {
+; CHECK-LABEL: vector_reverse_longint
----------------
Same comment as in previous function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95363