[PATCH] D107284: [LoopVectorize] Add support for replication of more intrinsics with scalable vectors

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8955
+    //   1. For the assume intrinsic generating the instruction for the first
+    //      lane may still be better than not generating any at all. For
+    //      example, the input may be a splat across all lanes.
----------------
`may` here makes it sound like it is not clear if it is better. The patch/code decides it is better, perhaps make that clear in the wording?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8960
+    //      suggests it should always be uniform. In the case when it is not
+    //      from an alloca, then the intrinsic would have no effect anyway.
+    return IsScalable || OrigLoop->hasLoopInvariantOperands(CI);
----------------
Can you keep the reasoning along here in line with `LangRef`? There's no reference to `alloca` there, just stack objects. Also, if it is not a stack object, the intrinsic still has an effect: poisoning the object. This should still allow to remove the call.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8979
+  if (!IsUniform && isa<CallInst>(I))
+    IsUniform =
+        isUniformIntrinsicCall(cast<CallInst>(I), Range.Start.isScalable());
----------------
can you add a comment here saying why we need to do another check?


================
Comment at: llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h:176
+  /// also uses the \p IsScalable flag to determine the result.
+  bool isUniformIntrinsicCall(CallInst *CI, bool IsScalable) const;
+
----------------
this only supports intrinsics, can it take `IntrinsicInst`?


================
Comment at: llvm/test/Transforms/LoopVectorize/scalable-assume.ll:90
+  %cmp15 = icmp eq i32 %n, 0
+  br i1 %cmp15, label %for.cond.cleanup, label %for.body.preheader
+
----------------
is this needed?(same for some other tests)


================
Comment at: llvm/test/Transforms/LoopVectorize/scalable-assume.ll:93
+for.body.preheader:                               ; preds = %entry
+  %0 = zext i32 %n to i64
+  br label %for.body
----------------
is this needed_


================
Comment at: llvm/test/Transforms/LoopVectorize/scalable-assume.ll:97
+for.cond.cleanup.loopexit:                        ; preds = %if.end5
+  br label %for.cond.cleanup
+
----------------
is this needed?


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

https://reviews.llvm.org/D107284