[PATCH] D106164: [LV] Don't assume isScalarAfterVectorization if one of the uses needs widening.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5148
+    // PossibleNonScalarPtrs.
+    if (llvm::all_of(I->users(), [&](User *U) {
+          return (isa<LoadInst>(U) || isa<StoreInst>(U)) &&
----------------
This change should not be needed I think; the changes above should be sufficient.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-widen-gep.ll:56
+  %curchar = phi i8** [ %curchar.next, %loop.body ], [ %curptr, %entry ]
+  %0 = phi i8* [ %incdec.ptr190, %loop.body ], [ %src, %entry ]
+  %incdec.ptr190 = getelementptr inbounds i8, i8* %0, i64 1
----------------
I think would be good to either assign more meaningful names to make the test a bit easier to read or use the same version as in `llvm/test/Transforms/LoopVectorize/pointer-induction.ll`.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-widen-gep.ll:4
+
+target triple = "aarch64-unknown-linux-gnu"
+
----------------
sdesmalen wrote:
> fhahn wrote:
> > Do we still need this test? The issue is not  SVE/AArch64 specific and it should already be covered by the general Test in Pointer-induction.ll.
> I'm a bit cautious about someone just regenerating the pointer-induction.ll test and reviewers not realising this is a big functional regression, because for scalable vectors it actually causes something to break. This test would guard that.
ok fair enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106164