Fix incorrect TypeSize->uint64_t cast in InductionDescriptor::isInductionPHI

[david-arm]

Summary

The code was relying upon the implicit conversion of TypeSize to
uint64_t and assuming the type in question was always fixed. However,
I discovered an issue when running the canon-freeze pass with some
IR loops that contains scalable vector types. I've changed the code
to bail out if the size is unknown at compile time, since we cannot
compute whether the step is a multiple of the type size or not.

I added a test here:

Transforms/CanonicalizeFreezeInLoops/phis.ll

[CarolineConcatto]

It looks like a sensible patch.
I don't see a reason why it should not be approved as it is.

[sdesmalen]

The static_cast to int64_t was indeed incorrect and bailing out seems the right thing to do.

I think it's worth adding a test to llvm/unittests/Analysis/IVDescriptorsTest.cpp as well and testing the call to isInductionPHI explicitly.

[david-arm]

• Added a unit test!

[sdesmalen]

LGTM with comments addressed.

llvm/lib/Analysis/IVDescriptors.cpp

1432

nit: I think this case could be supported, although that would require more work. Maybe you can leave a comment to say why we bail out for scalable vectors, and suggest there may be more work we can do here.

llvm/test/Transforms/CanonicalizeFreezeInLoops/phis.ll

116 ↗

(On Diff #406414)

This test can probably be removed now? I don't think testing the code for this specific pass serves any specific purpose now that you've got the unittest.

llvm/lib/Analysis/IVDescriptors.cpp

bool InductionDescriptor::isInductionPHI(
   Type *ElementType = PtrTy->isOpaque()
                           ? Type::getInt8Ty(PtrTy->getContext())
                           : PtrTy->getNonOpaquePointerElementType();
   if (!ElementType->isSized())
     return false;
 
   ConstantInt *CV = ConstStep->getValue();
   const DataLayout &DL = Phi->getModule()->getDataLayout();
-  int64_t Size = static_cast<int64_t>(DL.getTypeAllocSize(ElementType));
-  if (!Size)
+  TypeSize TySize = DL.getTypeAllocSize(ElementType);
+  // TODO: We could potentially support this for scalable vectors if we can
+  // prove at compile time that the constant step is always a multiple of
+  // the scalable type.
+  if (TySize.isZero() || TySize.isScalable())
     return false;
 
+  int64_t Size = static_cast<int64_t>(TySize.getFixedSize());
   int64_t CVSize = CV->getSExtValue();
   if (CVSize % Size)
     return false;
   auto *StepValue =
       SE->getConstant(CV->getType(), CVSize / Size, true /* signed */);
   D = InductionDescriptor(StartValue, IK_PtrInduction, StepValue,
                           /* BinOp */ nullptr, ElementType);
   return true;
 }

llvm/unittests/Analysis/IVDescriptorsTest.cpp

runWithLoopInfoAndSE(
           ++BBI;
         } while (!Inst_inc);
         assert(Inst_inc->getName() == "inc");
         EXPECT_EQ(IndDesc.getInductionBinOp(), Inst_inc);
         EXPECT_EQ(IndDesc.getExactFPMathInst(), nullptr);
       });
 }
 
+TEST(IVDescriptorsTest, LoopWithScalableTypes) {
+  // Parse the module.
+  LLVMContext Context;
+
+  std::unique_ptr<Module> M =
+      parseIR(Context,
+              R"(define void @foo(<vscale x 4 x float>* %ptr) {
+entry:
+  br label %for.body
+
+for.body:
+  %lsr.iv1 = phi <vscale x 4 x float>* [ %0, %for.body ], [ %ptr, %entry ]
+  %j.0117 = phi i64 [ %inc, %for.body ], [ 0, %entry ]
+  %lsr.iv12 = bitcast <vscale x 4 x float>* %lsr.iv1 to i8*
+  %inc = add nuw nsw i64 %j.0117, 1
+  %uglygep = getelementptr i8, i8* %lsr.iv12, i64 4
+  %0 = bitcast i8* %uglygep to <vscale x 4 x float>*
+  %cmp = icmp ne i64 %inc, 1024
+  br i1 %cmp, label %for.body, label %end
+
+end:
+  ret void
+})");
+
+  runWithLoopInfoAndSE(
+      *M, "foo", [&](Function &F, LoopInfo &LI, ScalarEvolution &SE) {
+        Function::iterator FI = F.begin();
+        // First basic block is entry - skip it.
+        BasicBlock *Header = &*(++FI);
+        assert(Header->getName() == "for.body");
+        Loop *L = LI.getLoopFor(Header);
+        EXPECT_NE(L, nullptr);
+        PHINode *Inst_iv = dyn_cast<PHINode>(&Header->front());
+        assert(Inst_iv->getName() == "lsr.iv1");
+        InductionDescriptor IndDesc;
+        bool IsInductionPHI =
+            InductionDescriptor::isInductionPHI(Inst_iv, L, &SE, IndDesc);
+        EXPECT_FALSE(IsInductionPHI);
+      });
+}
+
 // Depending on how SCEV deals with ptrtoint cast, the step of a phi could be
 // a pointer, and InductionDescriptor used to fail with an assertion.
 // So just check that it doesn't assert.
 TEST(IVDescriptorsTest, LoopWithPtrToInt) {
   // Parse the module.
   LLVMContext Context;
 
   std::unique_ptr<Module> M = parseIR(Context, R"(