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

[sdesmalen]

Summary

This fixes an issue that was found in D105199, where a GEP instruction
is used both as the address of a store, as well as the value of a store.
For the former, the value is scalar after vectorization, but the latter
(as value) requires widening.

Other code in that function seems to prevent similar cases from happening,
but it seems this case was missed.

[fhahn]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

5208

I think the actual problem is here.

Update should be inserted into ScalarPtrs. That way it will be only inserted into the work list if it is not identified as possibly non scalar. The comment above would also need updating accordingly. In this specific case, we should not treat pointer induction updates as scalar due to non-scalar uses.

[sdesmalen]

Instead of removing a scalar pointer from the worklist, prevent it from being added.

[sdesmalen]

Please ignore the previous revision :)

Changed patch to use ScalarPtrs and updated comment.

[david-arm]

LGTM! Could you address the nit before merging? Thanks. :)

llvm/test/Transforms/LoopVectorize/AArch64/sve-widen-gep.ll

79

nit: I think you can delete !4 -> !7 now.

[fhahn]

llvm/test/Transforms/LoopVectorize/AArch64/sve-widen-gep.ll

5

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.

[sdesmalen]

llvm/test/Transforms/LoopVectorize/AArch64/sve-widen-gep.ll

5

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.

[fhahn]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

5232

This change should not be needed I think; the changes above should be sufficient.

llvm/test/Transforms/LoopVectorize/AArch64/sve-widen-gep.ll

5

ok fair enough.

57

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.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

void LoopVectorizationCostModel::collectLoopScalars(ElementCount VF) {
   // inserted into Worklist. If the use will be a scalar use, and the
   // pointer is only used by memory accesses, we place the pointer in
   // ScalarPtrs. Otherwise, the pointer is placed in PossibleNonScalarPtrs.
   auto evaluatePtrUse = [&](Instruction *MemAccess, Value *Ptr) {
     if (isScalarPtrInduction(MemAccess, Ptr)) {
       Worklist.insert(cast<Instruction>(Ptr));
       Instruction *Update = cast<Instruction>(
           cast<PHINode>(Ptr)->getIncomingValueForBlock(Latch));
       Worklist.insert(Update);
       LLVM_DEBUG(dbgs() << "LV: Found new scalar instruction: " << *Ptr
                         << "\n");
       LLVM_DEBUG(dbgs() << "LV: Found new scalar instruction: " << *Update
                         << "\n");
       return;
     }
     // We only care about bitcast and getelementptr instructions contained in
     // the loop.
     if (!isLoopVaryingBitCastOrGEP(Ptr))
       return;
 
     // If the pointer has already been identified as scalar (e.g., if it was
     // also identified as uniform), there's nothing to do.
     auto *I = cast<Instruction>(Ptr);
     if (Worklist.count(I))
       return;
 
     // If the use of the pointer will be a scalar use, and all users of the
     // pointer are memory accesses, place the pointer in ScalarPtrs. Otherwise,
     // place the pointer in PossibleNonScalarPtrs.
-    if (isScalarUse(MemAccess, Ptr) && llvm::all_of(I->users(), [&](User *U) {
-          return isa<LoadInst>(U) || isa<StoreInst>(U);
+    if (llvm::all_of(I->users(), [&](User *U) {
+          return (isa<LoadInst>(U) || isa<StoreInst>(U)) &&
+                 isScalarUse(cast<Instruction>(U), Ptr);
         }))
       ScalarPtrs.insert(I);
     else
       PossibleNonScalarPtrs.insert(I);
   };
 
   // We seed the scalars analysis with three classes of instructions: (1)
   // instructions marked uniform-after-vectorization and (2) bitcast,
   // getelementptr and (pointer) phi instructions used by memory accesses
for (auto &I : *BB) {
       }
     }
   for (auto *I : ScalarPtrs)
     if (!PossibleNonScalarPtrs.count(I)) {
       LLVM_DEBUG(dbgs() << "LV: Found scalar instruction: " << *I << "\n");
       Worklist.insert(I);
     }
 
+  for (auto *I : PossibleNonScalarPtrs)
+    if (Worklist.count(I)) {
+      LLVM_DEBUG(dbgs() << "LV: Removed scalar instruction: " << *I << "\n");
+      Worklist.remove(I);
+    }
+
   // Insert the forced scalars.
   // FIXME: Currently widenPHIInstruction() often creates a dead vector
   // induction variable when the PHI user is scalarized.
   auto ForcedScalar = ForcedScalars.find(VF);
   if (ForcedScalar != ForcedScalars.end())
     for (auto *I : ForcedScalar->second)
       Worklist.insert(I);
 

llvm/test/Transforms/LoopVectorize/AArch64/sve-widen-gep.ll

+; REQUIRES: asserts
+; RUN: opt -loop-vectorize -scalable-vectorization=on -S -mtriple=aarch64 -mattr=+sve -debug-only=loop-vectorize < %s 2>&1 | FileCheck %s
+
+target triple = "aarch64-unknown-linux-gnu"
+
+; In the test below the PHI instruction:
+;  %0 = phi i8* [ %incdec.ptr190, %loop.body ], [ %src, %entry ]
+; has multiple uses, i.e.
+;  1. As a uniform address for the load, and
+;  2. Non-uniform use by the getelementptr + store, which leads to replication.
+
+; CHECK-LABEL:  LV: Checking a loop in "phi_multiple_use"
+; CHECK:        LV: Removed scalar instruction:   %incdec.ptr190 = getelementptr inbounds i8, i8* %0, i64 1
+;
+; CHECK:        VPlan 'Initial VPlan for VF={vscale x 2},UF>=1' {
+; CHECK-NEXT:   loop.body:
+; CHECK-NEXT:     WIDEN-INDUCTION %index = phi 0, %index.next
+; CHECK-NEXT:     WIDEN-PHI %curchar = phi %curchar.next, %curptr
+; CHECK-NEXT:     WIDEN-PHI %0 = phi %incdec.ptr190, %src
+; CHECK-NEXT:     WIDEN-GEP Var[Inv] ir<%incdec.ptr190> = getelementptr ir<%0>, ir<1>
+; CHECK-NEXT:     WIDEN store ir<%curchar>, ir<%incdec.ptr190>
+; CHECK-NEXT:     WIDEN ir<%1> = load ir<%0>
+; CHECK-NEXT:     WIDEN ir<%2> = add ir<%1>, ir<1>
+; CHECK-NEXT:     WIDEN store ir<%0>, ir<%2>
+; CHECK-NEXT:   No successors
+; CHECK-NEXT:   }
+
+define void @phi_multiple_use(i8** noalias %curptr, i8* noalias %src, i64 %N) #0 {
+; CHECK-LABEL: @phi_multiple_use(
+; CHECK:       vector.body:
+; CHECK-NEXT:    [[INDEX1:%.*]] = phi i64 [ 0, %vector.ph ], [ {{.*}}, %vector.body ]
+; CHECK-NEXT:    {{.*}} = add i64 [[INDEX1]], 0
+; CHECK-NEXT:    [[TMP1:%.*]] = add i64 [[INDEX1]], 0
+; CHECK-NEXT:    [[NEXT_GEP:%.*]] = getelementptr i8*, i8** %curptr, i64 [[TMP1]]
+; CHECK-NEXT:    [[TMP2:%.*]] = call <vscale x 2 x i64> @llvm.experimental.stepvector.nxv2i64()
+; CHECK-NEXT:    [[DOTSPLATINSERT:%.*]] = insertelement <vscale x 2 x i64> poison, i64 [[INDEX1]], i32 0
+; CHECK-NEXT:    [[DOTSPLAT:%.*]] = shufflevector <vscale x 2 x i64> [[DOTSPLATINSERT]], <vscale x 2 x i64> poison, <vscale x 2 x i32> zeroinitializer
+; CHECK-NEXT:    [[TMP3:%.*]] = add <vscale x 2 x i64> shufflevector (<vscale x 2 x i64> insertelement (<vscale x 2 x i64> poison, i64 0, i32 0), <vscale x 2 x i64> poison, <vscale x 2 x i32> zeroinitializer), [[TMP2]]
+; CHECK-NEXT:    [[TMP4:%.*]] = add <vscale x 2 x i64> [[DOTSPLAT]], [[TMP3]]
+; CHECK-NEXT:    [[NEXT_GEP6:%.*]] = getelementptr i8, i8* %src, <vscale x 2 x i64> [[TMP4]]
+; CHECK-NEXT:    [[TMP5:%.*]] = getelementptr inbounds i8, <vscale x 2 x i8*> [[NEXT_GEP6]], i64 1
+; CHECK:         store <vscale x 2 x i8*> [[TMP5]], <vscale x 2 x i8*>*
+; CHECK-NEXT:    [[TMP6:%.*]] = extractelement <vscale x 2 x i8*> [[NEXT_GEP6]], i32 0
+; CHECK-NEXT:    [[TMP7:%.*]] = getelementptr i8, i8* [[TMP6]], i32 0
+; CHECK-NEXT:    [[TMP8:%.*]] = bitcast i8* [[TMP7]] to <vscale x 2 x i8>*
+; CHECK-NEXT:    [[WIDE_LOAD:%.*]] = load <vscale x 2 x i8>, <vscale x 2 x i8>* [[TMP8]]
+; CHECK-NEXT:    [[TMP9:%.*]] = add <vscale x 2 x i8> [[WIDE_LOAD]],
+; CHECK:         store <vscale x 2 x i8> [[TMP9]], <vscale x 2 x i8>*
+
+entry:
+  br label %loop.body
+
+loop.body:                                    ; preds = %loop.body, %entry
+  %index = phi i64 [ 0, %entry ], [ %index.next, %loop.body ]
+  %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
+  %curchar.next = getelementptr inbounds i8*, i8** %curchar, i64 1
+  store i8* %incdec.ptr190, i8** %curchar, align 8
+  %1 = load i8, i8* %0, align 1
+  %2 = add i8 %1, 1
+  store i8 %2, i8* %0, align 1
+  %index.next = add nuw i64 %index, 1
+  %3 = icmp ne i64 %index.next, %N
+  br i1 %3, label %loop.body, label %exit, !llvm.loop !0
+
+exit:                            ; preds = %loop.body
+  ret void
+}
+
+
+attributes #0 = {"target-features"="+sve"}
+
+!0 = distinct !{!0, !1, !2, !3}
+!1 = !{!"llvm.loop.interleave.count", i32 1}
+!2 = !{!"llvm.loop.vectorize.width", i32 2}
+!3 = !{!"llvm.loop.vectorize.scalable.enable", i1 true}
+!4 = !{ !5 }
+!5 = distinct !{ !5, !6 }
+!6 = distinct !{ !7 }
+!7 = distinct !{ !7, !6 }