Skip to content

Commit

Permalink
Fix incorrect TypeSize->uint64_t cast in InductionDescriptor::isInduc…
Browse files Browse the repository at this point in the history
…tionPHI

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

Differential Revision: https://reviews.llvm.org/D118696

(cherry picked from commit 1badfbb)
  • Loading branch information
david-arm authored and tstellar committed Feb 22, 2022
1 parent 8c33ea3 commit 8b5b29c
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 2 deletions.
8 changes: 6 additions & 2 deletions llvm/lib/Analysis/IVDescriptors.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1428,10 +1428,14 @@ bool InductionDescriptor::isInductionPHI(

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;
Expand Down
41 changes: 41 additions & 0 deletions llvm/unittests/Analysis/IVDescriptorsTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,47 @@ for.end:
});
}

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.
Expand Down

0 comments on commit 8b5b29c

Please sign in to comment.