Problem:
for.cond2106: ; preds = %for.cond2106, %entry %gid.0 = phi i8* [ null, %entry ], [ %incdec.ptr, %for.cond2106 ] %i.0 = phi i32 [ 0, %entry ], [ %inc2117, %for.cond2106 ] %incdec.ptr = getelementptr inbounds i8, i8* %gid.0, i64 1 %idxprom2114 = zext i32 %i.0 to i64 %inc2117 = add nuw nsw i32 %i.0, 1 br label %for.cond2106
IndVarSimplify wants to eliminate %idxprom2114 = zext i32 %i.0 to i64, so it tries to widen PHI node for %I.0 from i32 to i64.
When IndVarSimplify tries to expand code for above phi node’s SCEV {0, +, 1} in createWideIV, scevexpander finds an existing instruction with same SCEV, the first PHI node %gid.0.
Then when scevexpander adds a cast from first PHI node type i8 * to i64, we get assert inside ReuseOrCreateCast.
Root Cause:
Reason for the assert is the cast instruction is generated at insert point which ignores all PHIs, but when IndVarSimplify starts to expand the SCEV for the PHI, it sets the insert point at the loop header’s very beginning.
So the new created cast(ptrtoint) does not dominates the expected use place. (insert point set by IndVarSimplify, which is the loop header’s very beginning).
Solution:
This is the second issue exposed by the insert point setting by IndVarSimplify:
// The rewriter provides a value for the desired IV expression. This may // either find an existing phi or materialize a new one. Either way, we // expect a well-formed cyclic phi-with-increments. i.e. any operand not part // of the phi-SCC dominates the loop entry. Instruction *InsertPt = &L->getHeader()->front(); WidePhi = cast<PHINode>(Rewriter.expandCodeFor(AddRec, WideType, InsertPt));
The first one was fixed in https://reviews.llvm.org/D57428, it corrects the insert point for IndVarSimplify specially in Value *SCEVExpander::expand as:
// IndVarSimplify sometimes sets the insertion point at the block start, even // when there are PHIs at that point. We must correct for this. if (isa<PHINode>(*InsertPt)) InsertPt = &*InsertPt->getParent()->getFirstInsertionPt();
This patch also fixes the insert point issue like above.
Improved solution?:
But should we fix it in IndVarSimplify instead of SCEVExpander? The author for https://reviews.llvm.org/D57428 also tried this way, but he found there are other places also set the insert point at the loop header’s very beginning, like the following statements in SCEVExpander::getAddRecExprPHILiterally:
// Expand the step somewhere that dominates the loop header. Value *StepV = expandCodeFor(Step, IntTy, &L->getHeader()->front());
I went through llvm code base, there are other places calling expandCodeFor with insert point L->getHeader()->front(), I guess they are all not safe if the first instruction at the header front is PHI and the instruction we expand is not a PHI node?
Do we need to fix all these places? and one thing is there are many places whose insert points are parameters from their caller, so for this case we can not avoid the risk?
May be we need to add some check in Value *SCEVExpander::expandCodeFor(const SCEV *SH, Type *Ty, Instruction *IP) like below, so we don’t need to go through all expandCodeFor callings and also can remove the fix for the insert point deep inside SCEVExpander?
Value *SCEVExpander::expandCodeFor(const SCEV *SH, Type *Ty,
Instruction *IP) {
if (isa<PHINode>(*IP))
IP = InsertPt = &*InsertPt->getParent()->getFirstInsertionPt();
setInsertPoint(IP);
return expandCodeFor(SH, Ty);
}This should also resolve https://bugs.llvm.org/show_bug.cgi?id=37004
Thanks in advance for your comments.
the expandCodeFor always modifies current function, but returning nullptr is interpreted as « no change ». This makes pass return status invalid (the pass return false while a change actually happened).
This breaks validation http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-debian/builds/9441/, and there wasn't a simple patch.