[llvm] 965a090 - Revert "[IVDescriptors] Add pointer InductionDescriptors with non-constant strides"

Philip Reames via llvm-commits llvm-commits at lists.llvm.org
Fri Mar 31 08:28:26 PDT 2023


Thank you for the revert.  Will follow up in the commit thread, and a 
new review.  With this much fallout, this clearly isn't "obvious".

Also, JFYI, commenting on the phabricator commit is going to be slower 
to get a response from me than replying to the commit thread.  I watch 
the commit threads on llvm-commits fairly closely, but the way 
phabricator sends the notices on a commit thread get lost with all of 
the other low value phab emails.

Philip

On 3/31/23 03:08, David Green via llvm-commits wrote:
> Author: David Green
> Date: 2023-03-31T11:08:50+01:00
> New Revision: 965a090f02cbab2dc08b78e274ce9d65e4d1221f
>
> URL: https://github.com/llvm/llvm-project/commit/965a090f02cbab2dc08b78e274ce9d65e4d1221f
> DIFF: https://github.com/llvm/llvm-project/commit/965a090f02cbab2dc08b78e274ce9d65e4d1221f.diff
>
> LOG: Revert "[IVDescriptors] Add pointer InductionDescriptors with non-constant strides"
>
> Multiple errors have being reported on
> https://reviews.llvm.org/rG498aa534f472d28db893aa9a8627d0b46e17f312
>
> Reverting until the correctness issues can be resolved.
>
> We are also seeing a lot of performance differences from the patch.  Some are
> looking good, but some are looking pretty bad.
>
> Added:
>      
>
> Modified:
>      llvm/lib/Analysis/IVDescriptors.cpp
>      llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
>      llvm/test/Transforms/LoopVectorize/RISCV/strided-accesses.ll
>
> Removed:
>      
>
>
> ################################################################################
> diff  --git a/llvm/lib/Analysis/IVDescriptors.cpp b/llvm/lib/Analysis/IVDescriptors.cpp
> index 22b9f64ef88d8..156d141c461ed 100644
> --- a/llvm/lib/Analysis/IVDescriptors.cpp
> +++ b/llvm/lib/Analysis/IVDescriptors.cpp
> @@ -1288,6 +1288,8 @@ InductionDescriptor::InductionDescriptor(Value *Start, InductionKind K,
>     assert((!getConstIntStepValue() || !getConstIntStepValue()->isZero()) &&
>            "Step value is zero");
>   
> +  assert((IK != IK_PtrInduction || getConstIntStepValue()) &&
> +         "Step value should be constant for pointer induction");
>     assert((IK == IK_FpInduction || Step->getType()->isIntegerTy()) &&
>            "StepValue is not an integer");
>   
> @@ -1568,25 +1570,15 @@ bool InductionDescriptor::isInductionPHI(
>     }
>   
>     assert(PhiTy->isPointerTy() && "The PHI must be a pointer");
> -  PointerType *PtrTy = cast<PointerType>(PhiTy);
> -
> -  // Always use i8 element type for opaque pointer inductions.
> -  // This allows induction variables w/non-constant steps.
> -  if (PtrTy->isOpaque()) {
> -    D = InductionDescriptor(StartValue, IK_PtrInduction, Step,
> -                            /* BinOp */ nullptr,
> -                            Type::getInt8Ty(PtrTy->getContext()));
> -    return true;
> -  }
> -
>     // Pointer induction should be a constant.
> -  // TODO: This could be generalized, but should probably just
> -  // be dropped instead once the migration to opaque ptrs is
> -  // complete.
>     if (!ConstStep)
>       return false;
>   
> -  Type *ElementType = PtrTy->getNonOpaquePointerElementType();
> +  // Always use i8 element type for opaque pointer inductions.
> +  PointerType *PtrTy = cast<PointerType>(PhiTy);
> +  Type *ElementType = PtrTy->isOpaque()
> +                          ? Type::getInt8Ty(PtrTy->getContext())
> +                          : PtrTy->getNonOpaquePointerElementType();
>     if (!ElementType->isSized())
>       return false;
>   
>
> diff  --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
> index df7c430d326af..21858cc1705b6 100644
> --- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
> +++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
> @@ -2492,6 +2492,8 @@ static Value *emitTransformedIndex(IRBuilderBase &B, Value *Index,
>       return CreateAdd(StartValue, Offset);
>     }
>     case InductionDescriptor::IK_PtrInduction: {
> +    assert(isa<Constant>(Step) &&
> +           "Expected constant step for pointer induction");
>       return B.CreateGEP(ID.getElementType(), StartValue, CreateMul(Index, Step));
>     }
>     case InductionDescriptor::IK_FpInduction: {
> @@ -8269,6 +8271,7 @@ VPRecipeBase *VPRecipeBuilder::tryToOptimizeInductionPHI(
>     if (auto *II = Legal->getPointerInductionDescriptor(Phi)) {
>       VPValue *Step = vputils::getOrCreateVPValueForSCEVExpr(Plan, II->getStep(),
>                                                              *PSE.getSE());
> +    assert(isa<SCEVConstant>(II->getStep()));
>       return new VPWidenPointerInductionRecipe(
>           Phi, Operands[0], Step, *II,
>           LoopVectorizationPlanner::getDecisionAndClampRange(
> @@ -9374,6 +9377,8 @@ void VPWidenPointerInductionRecipe::execute(VPTransformState &State) {
>       return;
>     }
>   
> +  assert(isa<SCEVConstant>(IndDesc.getStep()) &&
> +         "Induction step not a SCEV constant!");
>     Type *PhiType = IndDesc.getStep()->getType();
>   
>     // Build a pointer phi
>
> diff  --git a/llvm/test/Transforms/LoopVectorize/RISCV/strided-accesses.ll b/llvm/test/Transforms/LoopVectorize/RISCV/strided-accesses.ll
> index 0ba05c1d621ae..a108e9b0129ce 100644
> --- a/llvm/test/Transforms/LoopVectorize/RISCV/strided-accesses.ll
> +++ b/llvm/test/Transforms/LoopVectorize/RISCV/strided-accesses.ll
> @@ -539,89 +539,19 @@ exit:
>   define void @double_stride_ptr_iv(ptr %p, ptr %p2, i64 %stride) {
>   ; CHECK-LABEL: @double_stride_ptr_iv(
>   ; CHECK-NEXT:  entry:
> -; CHECK-NEXT:    [[TMP0:%.*]] = call i64 @llvm.vscale.i64()
> -; CHECK-NEXT:    [[TMP1:%.*]] = mul i64 [[TMP0]], 4
> -; CHECK-NEXT:    [[TMP2:%.*]] = call i64 @llvm.umax.i64(i64 8, i64 [[TMP1]])
> -; CHECK-NEXT:    [[MIN_ITERS_CHECK:%.*]] = icmp ult i64 1024, [[TMP2]]
> -; CHECK-NEXT:    br i1 [[MIN_ITERS_CHECK]], label [[SCALAR_PH:%.*]], label [[VECTOR_SCEVCHECK:%.*]]
> -; CHECK:       vector.scevcheck:
> -; CHECK-NEXT:    [[IDENT_CHECK:%.*]] = icmp ne i64 [[STRIDE:%.*]], 1
> -; CHECK-NEXT:    br i1 [[IDENT_CHECK]], label [[SCALAR_PH]], label [[VECTOR_MEMCHECK:%.*]]
> -; CHECK:       vector.memcheck:
> -; CHECK-NEXT:    [[SCEVGEP:%.*]] = getelementptr i8, ptr [[P2:%.*]], i64 1027
> -; CHECK-NEXT:    [[SCEVGEP1:%.*]] = getelementptr i8, ptr [[P:%.*]], i64 1027
> -; CHECK-NEXT:    [[BOUND0:%.*]] = icmp ult ptr [[P2]], [[SCEVGEP1]]
> -; CHECK-NEXT:    [[BOUND1:%.*]] = icmp ult ptr [[P]], [[SCEVGEP]]
> -; CHECK-NEXT:    [[FOUND_CONFLICT:%.*]] = and i1 [[BOUND0]], [[BOUND1]]
> -; CHECK-NEXT:    br i1 [[FOUND_CONFLICT]], label [[SCALAR_PH]], label [[VECTOR_PH:%.*]]
> -; CHECK:       vector.ph:
> -; CHECK-NEXT:    [[TMP3:%.*]] = call i64 @llvm.vscale.i64()
> -; CHECK-NEXT:    [[TMP4:%.*]] = mul i64 [[TMP3]], 4
> -; CHECK-NEXT:    [[N_MOD_VF:%.*]] = urem i64 1024, [[TMP4]]
> -; CHECK-NEXT:    [[N_VEC:%.*]] = sub i64 1024, [[N_MOD_VF]]
> -; CHECK-NEXT:    [[TMP5:%.*]] = mul i64 [[N_VEC]], [[STRIDE]]
> -; CHECK-NEXT:    [[IND_END:%.*]] = getelementptr i8, ptr [[P]], i64 [[TMP5]]
> -; CHECK-NEXT:    [[TMP6:%.*]] = mul i64 [[N_VEC]], [[STRIDE]]
> -; CHECK-NEXT:    [[IND_END3:%.*]] = getelementptr i8, ptr [[P2]], i64 [[TMP6]]
> -; CHECK-NEXT:    br label [[VECTOR_BODY:%.*]]
> -; CHECK:       vector.body:
> -; CHECK-NEXT:    [[POINTER_PHI:%.*]] = phi ptr [ [[P]], [[VECTOR_PH]] ], [ [[PTR_IND:%.*]], [[VECTOR_BODY]] ]
> -; CHECK-NEXT:    [[POINTER_PHI7:%.*]] = phi ptr [ [[P2]], [[VECTOR_PH]] ], [ [[PTR_IND8:%.*]], [[VECTOR_BODY]] ]
> -; CHECK-NEXT:    [[INDEX:%.*]] = phi i64 [ 0, [[VECTOR_PH]] ], [ [[INDEX_NEXT:%.*]], [[VECTOR_BODY]] ]
> -; CHECK-NEXT:    [[TMP7:%.*]] = call i64 @llvm.vscale.i64()
> -; CHECK-NEXT:    [[TMP8:%.*]] = mul i64 [[TMP7]], 4
> -; CHECK-NEXT:    [[TMP9:%.*]] = mul i64 [[TMP8]], 1
> -; CHECK-NEXT:    [[TMP10:%.*]] = mul i64 [[STRIDE]], [[TMP9]]
> -; CHECK-NEXT:    [[TMP11:%.*]] = mul i64 [[TMP8]], 0
> -; CHECK-NEXT:    [[DOTSPLATINSERT:%.*]] = insertelement <vscale x 4 x i64> poison, i64 [[TMP11]], i64 0
> -; CHECK-NEXT:    [[DOTSPLAT:%.*]] = shufflevector <vscale x 4 x i64> [[DOTSPLATINSERT]], <vscale x 4 x i64> poison, <vscale x 4 x i32> zeroinitializer
> -; CHECK-NEXT:    [[TMP12:%.*]] = call <vscale x 4 x i64> @llvm.experimental.stepvector.nxv4i64()
> -; CHECK-NEXT:    [[TMP13:%.*]] = add <vscale x 4 x i64> [[DOTSPLAT]], [[TMP12]]
> -; CHECK-NEXT:    [[DOTSPLATINSERT5:%.*]] = insertelement <vscale x 4 x i64> poison, i64 [[STRIDE]], i64 0
> -; CHECK-NEXT:    [[DOTSPLAT6:%.*]] = shufflevector <vscale x 4 x i64> [[DOTSPLATINSERT5]], <vscale x 4 x i64> poison, <vscale x 4 x i32> zeroinitializer
> -; CHECK-NEXT:    [[VECTOR_GEP:%.*]] = mul <vscale x 4 x i64> [[TMP13]], [[DOTSPLAT6]]
> -; CHECK-NEXT:    [[TMP14:%.*]] = getelementptr i8, ptr [[POINTER_PHI]], <vscale x 4 x i64> [[VECTOR_GEP]]
> -; CHECK-NEXT:    [[TMP15:%.*]] = call i64 @llvm.vscale.i64()
> -; CHECK-NEXT:    [[TMP16:%.*]] = mul i64 [[TMP15]], 4
> -; CHECK-NEXT:    [[TMP17:%.*]] = mul i64 [[TMP16]], 1
> -; CHECK-NEXT:    [[TMP18:%.*]] = mul i64 [[STRIDE]], [[TMP17]]
> -; CHECK-NEXT:    [[TMP19:%.*]] = mul i64 [[TMP16]], 0
> -; CHECK-NEXT:    [[DOTSPLATINSERT9:%.*]] = insertelement <vscale x 4 x i64> poison, i64 [[TMP19]], i64 0
> -; CHECK-NEXT:    [[DOTSPLAT10:%.*]] = shufflevector <vscale x 4 x i64> [[DOTSPLATINSERT9]], <vscale x 4 x i64> poison, <vscale x 4 x i32> zeroinitializer
> -; CHECK-NEXT:    [[TMP20:%.*]] = call <vscale x 4 x i64> @llvm.experimental.stepvector.nxv4i64()
> -; CHECK-NEXT:    [[TMP21:%.*]] = add <vscale x 4 x i64> [[DOTSPLAT10]], [[TMP20]]
> -; CHECK-NEXT:    [[VECTOR_GEP13:%.*]] = mul <vscale x 4 x i64> [[TMP21]], [[DOTSPLAT6]]
> -; CHECK-NEXT:    [[TMP22:%.*]] = getelementptr i8, ptr [[POINTER_PHI7]], <vscale x 4 x i64> [[VECTOR_GEP13]]
> -; CHECK-NEXT:    [[WIDE_MASKED_GATHER:%.*]] = call <vscale x 4 x i32> @llvm.masked.gather.nxv4i32.nxv4p0(<vscale x 4 x ptr> [[TMP14]], i32 4, <vscale x 4 x i1> shufflevector (<vscale x 4 x i1> insertelement (<vscale x 4 x i1> poison, i1 true, i64 0), <vscale x 4 x i1> poison, <vscale x 4 x i32> zeroinitializer), <vscale x 4 x i32> poison), !alias.scope !16
> -; CHECK-NEXT:    [[TMP23:%.*]] = add <vscale x 4 x i32> [[WIDE_MASKED_GATHER]], shufflevector (<vscale x 4 x i32> insertelement (<vscale x 4 x i32> poison, i32 1, i64 0), <vscale x 4 x i32> poison, <vscale x 4 x i32> zeroinitializer)
> -; CHECK-NEXT:    call void @llvm.masked.scatter.nxv4i32.nxv4p0(<vscale x 4 x i32> [[TMP23]], <vscale x 4 x ptr> [[TMP22]], i32 4, <vscale x 4 x i1> shufflevector (<vscale x 4 x i1> insertelement (<vscale x 4 x i1> poison, i1 true, i64 0), <vscale x 4 x i1> poison, <vscale x 4 x i32> zeroinitializer)), !alias.scope !19, !noalias !16
> -; CHECK-NEXT:    [[TMP24:%.*]] = call i64 @llvm.vscale.i64()
> -; CHECK-NEXT:    [[TMP25:%.*]] = mul i64 [[TMP24]], 4
> -; CHECK-NEXT:    [[INDEX_NEXT]] = add nuw i64 [[INDEX]], [[TMP25]]
> -; CHECK-NEXT:    [[PTR_IND]] = getelementptr i8, ptr [[POINTER_PHI]], i64 [[TMP10]]
> -; CHECK-NEXT:    [[PTR_IND8]] = getelementptr i8, ptr [[POINTER_PHI7]], i64 [[TMP18]]
> -; CHECK-NEXT:    [[TMP26:%.*]] = icmp eq i64 [[INDEX_NEXT]], [[N_VEC]]
> -; CHECK-NEXT:    br i1 [[TMP26]], label [[MIDDLE_BLOCK:%.*]], label [[VECTOR_BODY]], !llvm.loop [[LOOP21:![0-9]+]]
> -; CHECK:       middle.block:
> -; CHECK-NEXT:    [[CMP_N:%.*]] = icmp eq i64 1024, [[N_VEC]]
> -; CHECK-NEXT:    br i1 [[CMP_N]], label [[EXIT:%.*]], label [[SCALAR_PH]]
> -; CHECK:       scalar.ph:
> -; CHECK-NEXT:    [[BC_RESUME_VAL:%.*]] = phi i64 [ [[N_VEC]], [[MIDDLE_BLOCK]] ], [ 0, [[ENTRY:%.*]] ], [ 0, [[VECTOR_SCEVCHECK]] ], [ 0, [[VECTOR_MEMCHECK]] ]
> -; CHECK-NEXT:    [[BC_RESUME_VAL2:%.*]] = phi ptr [ [[IND_END]], [[MIDDLE_BLOCK]] ], [ [[P]], [[ENTRY]] ], [ [[P]], [[VECTOR_SCEVCHECK]] ], [ [[P]], [[VECTOR_MEMCHECK]] ]
> -; CHECK-NEXT:    [[BC_RESUME_VAL4:%.*]] = phi ptr [ [[IND_END3]], [[MIDDLE_BLOCK]] ], [ [[P2]], [[ENTRY]] ], [ [[P2]], [[VECTOR_SCEVCHECK]] ], [ [[P2]], [[VECTOR_MEMCHECK]] ]
>   ; CHECK-NEXT:    br label [[LOOP:%.*]]
>   ; CHECK:       loop:
> -; CHECK-NEXT:    [[I:%.*]] = phi i64 [ [[BC_RESUME_VAL]], [[SCALAR_PH]] ], [ [[NEXTI:%.*]], [[LOOP]] ]
> -; CHECK-NEXT:    [[PTR:%.*]] = phi ptr [ [[BC_RESUME_VAL2]], [[SCALAR_PH]] ], [ [[PTR_NEXT:%.*]], [[LOOP]] ]
> -; CHECK-NEXT:    [[PTR2:%.*]] = phi ptr [ [[BC_RESUME_VAL4]], [[SCALAR_PH]] ], [ [[PTR2_NEXT:%.*]], [[LOOP]] ]
> +; CHECK-NEXT:    [[I:%.*]] = phi i64 [ 0, [[ENTRY:%.*]] ], [ [[NEXTI:%.*]], [[LOOP]] ]
> +; CHECK-NEXT:    [[PTR:%.*]] = phi ptr [ [[P:%.*]], [[ENTRY]] ], [ [[PTR_NEXT:%.*]], [[LOOP]] ]
> +; CHECK-NEXT:    [[PTR2:%.*]] = phi ptr [ [[P2:%.*]], [[ENTRY]] ], [ [[PTR2_NEXT:%.*]], [[LOOP]] ]
>   ; CHECK-NEXT:    [[X0:%.*]] = load i32, ptr [[PTR]], align 4
>   ; CHECK-NEXT:    [[Y0:%.*]] = add i32 [[X0]], 1
>   ; CHECK-NEXT:    store i32 [[Y0]], ptr [[PTR2]], align 4
> -; CHECK-NEXT:    [[PTR_NEXT]] = getelementptr inbounds i8, ptr [[PTR]], i64 [[STRIDE]]
> +; CHECK-NEXT:    [[PTR_NEXT]] = getelementptr inbounds i8, ptr [[PTR]], i64 [[STRIDE:%.*]]
>   ; CHECK-NEXT:    [[PTR2_NEXT]] = getelementptr inbounds i8, ptr [[PTR2]], i64 [[STRIDE]]
>   ; CHECK-NEXT:    [[NEXTI]] = add i64 [[I]], 1
>   ; CHECK-NEXT:    [[DONE:%.*]] = icmp eq i64 [[NEXTI]], 1024
> -; CHECK-NEXT:    br i1 [[DONE]], label [[EXIT]], label [[LOOP]], !llvm.loop [[LOOP22:![0-9]+]]
> +; CHECK-NEXT:    br i1 [[DONE]], label [[EXIT:%.*]], label [[LOOP]]
>   ; CHECK:       exit:
>   ; CHECK-NEXT:    ret void
>   ;
>
>
>          
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits


More information about the llvm-commits mailing list