[PATCH] D105199: [LoopVectorize] Fix scalable vector crash in VPReplicateRecipe::execute

[Caroline via Phabricator via llvm-commits]

CarolineConcatto added a comment.

Hey David,
Thank you for adding me as a reviewer. 
I try to add some comments that I believe are valid, hope they make sense.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3069
 
   // End if-block.
   if (IfPredicateInstr)
----------------
Do we need this test also for scalable recipe  in scalarizeInstructions 

```
  if (IfPredicateInstr)
```
?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8964-8965
+  bool IsScalable = Range.Start.isScalable();
+  assert(IsScalable == Range.End.isScalable() &&
+         "VFRange contains mixture of scalable and fixed-width VFs!");
+  VPRecipeBase *Recipe;
----------------
I believe you don't need the assert
See lib/Transforms/Vectorize/VPlan.h
struct VFRange;


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8966
+         "VFRange contains mixture of scalable and fixed-width VFs!");
+  VPRecipeBase *Recipe;
+  if (IsScalable && !IsUniform) {
----------------
Why do we need R and Recipe here?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8967
+  VPRecipeBase *Recipe;
+  if (IsScalable && !IsUniform) {
+    auto *R =
----------------
Should we test if this is not predicated instruction here?
I don't see any test for that in the recipe.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-vpreplicate.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -loop-vectorize -scalable-vectorization=on -S | FileCheck %s
+
----------------
Is it possible to use that with for all architectures? 
Or only  for now for AArch64?


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-vpreplicate.ll:35
+entry:
+  br label %while.body189
+
----------------
nit: 
s/while.body189/loop.body/
s/while.end192.loopexit/exit/


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-vpreplicate.ll:136
+!3 = !{!"llvm.loop.vectorize.scalable.enable", i1 true}
+!4 = !{ !5 }
+!5 = distinct !{ !5, !6 }
----------------
Why do you need 4,5,6 and 7?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105199