[LoopVectorize] Don't create unnecessary vscale intrinsic calls

[david-arm]

Summary

In quite a few cases in LoopVectorize.cpp we call createStepForVF
with a step value of 0, which leads to unnecessary generation of
llvm.vscale intrinsic calls. I've optimised IRBuilder::CreateVScale
and createStepForVF to return 0 when attempting to multiply
vscale by 0.

[frasercrmck]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

1116

Not that this is wrong, but would it be inappropriate to make CreateVScale detect a zero step and return zero?

[frasercrmck]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

1116

Sorry I just realised that's what you've done. Do we need to make this change, then?

[david-arm]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

1116

It's not really necessary as the end result is the same - I was just trying to avoid the compiler doing more work for the Step=0 case, i.e. the creation of a ConstantInt, etc. I noticed there are a few places in the vectoriser where we call this function with a Step of 0 - it happens when widening a PHI instruction or vectorising induction variables.

If you prefer I can just revert this code?

[frasercrmck]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

1116

I don't feel particularly strongly. I suppose my initial comment was trying to avoid repeating ourselves and duplicating logic. But I also realise that this is how compile-time regressions creep in. I don't know how impactful the creation of a zero ConstantInt is? Presumably it's cached more often than not?

[sdesmalen]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

1116

The overhead of creating the ConstantInt is small, and this only happens on a small number of cases when the step value is actually zero. Personally I'd prefer this to be implemented only once in IRBuilder, and have this change removed to make the code simpler.

[david-arm]

• Reverted change to createStepForVF

[sdesmalen]

Is it worth adding a test to llvm/unittests/IR/IRBuilderTest.cpp for this change?

[david-arm]

• Added unit test for vscale * 0 case.

[sdesmalen]

LGTM, thanks for adding the test.

[frasercrmck]

LGTM too, thanks.

llvm/lib/IR/IRBuilder.cpp

static CallInst *createCallHelper(Function *Callee, ArrayRef<Value *> Ops,
                                   ArrayRef<OperandBundleDef> OpBundles = {}) {
   CallInst *CI = Builder->CreateCall(Callee, Ops, OpBundles, Name);
   if (FMFSource)
     CI->copyFastMathFlags(FMFSource);
   return CI;
 }
 
 Value *IRBuilderBase::CreateVScale(Constant *Scaling, const Twine &Name) {
-  Module *M = GetInsertBlock()->getParent()->getParent();
   assert(isa<ConstantInt>(Scaling) && "Expected constant integer");
+  if (cast<ConstantInt>(Scaling)->isZero())
+    return Scaling;
+  Module *M = GetInsertBlock()->getParent()->getParent();
   Function *TheFn =
       Intrinsic::getDeclaration(M, Intrinsic::vscale, {Scaling->getType()});
   CallInst *CI = createCallHelper(TheFn, {}, this, Name);
   return cast<ConstantInt>(Scaling)->getSExtValue() == 1
              ? CI
              : CreateMul(CI, Scaling);
 }
 

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

static OptimizationRemarkAnalysis createLVAnalysis(const char *PassName,
   OptimizationRemarkAnalysis R(PassName, RemarkName, DL, CodeRegion);
   R << "loop not vectorized: ";
   return R;
 }
 
 /// Return a value for Step multiplied by VF.
 static Value *createStepForVF(IRBuilder<> &B, Constant *Step, ElementCount VF) {
   assert(isa<ConstantInt>(Step) && "Expected an integer step");
-  Constant *StepVal = ConstantInt::get(
-      Step->getType(),
-      cast<ConstantInt>(Step)->getSExtValue() * VF.getKnownMinValue());
-  return VF.isScalable() ? B.CreateVScale(StepVal) : StepVal;
+  uint64_t StepVal = cast<ConstantInt>(Step)->getSExtValue();
+  if (StepVal == 0)
+    return Step;
+  Constant *NewStep =
+      ConstantInt::get(Step->getType(), StepVal * VF.getKnownMinValue());
+  return VF.isScalable() ? B.CreateVScale(NewStep) : NewStep;
 }
 
 namespace llvm {
 
 /// Return the runtime value for VF.
 Value *getRuntimeVF(IRBuilder<> &B, Type *Ty, ElementCount VF) {
   Constant *EC = ConstantInt::get(Ty, VF.getKnownMinValue());
   return VF.isScalable() ? B.CreateVScale(EC) : EC;
if (Cost->isScalarAfterVectorization(P, State.VF)) {
       // Determine the number of scalars we need to generate for each unroll
       // iteration. If the instruction is uniform, we only need to generate the
       // first lane. Otherwise, we generate all VF values.
       bool IsUniform = Cost->isUniformAfterVectorization(P, State.VF);
       assert((IsUniform || !VF.isScalable()) &&
              "Currently unsupported for scalable vectors");
       unsigned Lanes = IsUniform ? 1 : State.VF.getFixedValue();
 
-      Value *RuntimeVF = getRuntimeVF(Builder, PtrInd->getType(), VF);
       for (unsigned Part = 0; Part < UF; ++Part) {
-        Value *PartStart = Builder.CreateMul(
-            RuntimeVF, ConstantInt::get(PtrInd->getType(), Part));
+        Value *PartStart = createStepForVF(
+            Builder, ConstantInt::get(PtrInd->getType(), Part), VF);
         for (unsigned Lane = 0; Lane < Lanes; ++Lane) {
           Value *Idx = Builder.CreateAdd(
               PartStart, ConstantInt::get(PtrInd->getType(), Lane));
           Value *GlobalIdx = Builder.CreateAdd(PtrInd, Idx);
           Value *SclrGep =
               emitTransformedIndex(Builder, GlobalIdx, PSE.getSE(), DL, II);
           SclrGep->setName("next.gep");
           State.set(PhiR, SclrGep, VPIteration(Part, Lane));