[PATCH] D80975: [SCEV][IndVarSimplify] insert point should not be block front if the front instruction is a PHI
ChenZheng via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 1 21:37:08 PDT 2020
shchenz created this revision.
shchenz added reviewers: sanjoy, reames, nikic, fhahn, mkazantsev, samparker.
Herald added subscribers: llvm-commits, wuzish, javed.absar, hiraditya.
Herald added a project: LLVM.
**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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D80975
Files:
llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
llvm/test/Transforms/IndVarSimplify/widen-i32-i8ptr.ll
Index: llvm/test/Transforms/IndVarSimplify/widen-i32-i8ptr.ll
===================================================================
--- /dev/null
+++ llvm/test/Transforms/IndVarSimplify/widen-i32-i8ptr.ll
@@ -0,0 +1,24 @@
+; RUN: opt < %s -indvars -S | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-n32:64"
+
+define dso_local void @Widen_i32_i8ptr() local_unnamed_addr {
+; CHECK-LABEL: @Widen_i32_i8ptr(
+; CHECK: phi i8*
+; CHECK: phi i32
+entry:
+ %ptrids = alloca [15 x i8*], align 8
+ %arraydecay2032 = getelementptr inbounds [15 x i8*], [15 x i8*]* %ptrids, i64 0, i64 0
+ store i8** %arraydecay2032, i8*** inttoptr (i64 8 to i8***), align 8
+ br label %for.cond2106
+
+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
+ %arrayidx2115 = getelementptr inbounds [15 x i8*], [15 x i8*]* %ptrids, i64 0, i64 %idxprom2114
+ store i8* %gid.0, i8** %arrayidx2115, align 8
+ %inc2117 = add nuw nsw i32 %i.0, 1
+ br label %for.cond2106
+}
Index: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
===================================================================
--- llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
+++ llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
@@ -53,7 +53,12 @@
// Since we don't know the builder's insertion point is actually
// where the uses will be added (only that it dominates it), we are
// not allowed to move it.
- BasicBlock::iterator BIP = Builder.GetInsertPoint();
+ Instruction *BIP = &*Builder.GetInsertPoint();
+
+ // 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>(*BIP))
+ BIP = &*BIP->getParent()->getFirstNonPHI();
Instruction *Ret = nullptr;
@@ -65,7 +70,7 @@
// If the cast isn't where we want it, create a new cast at IP.
// Likewise, do not reuse a cast at BIP because it must dominate
// instructions that might be inserted before BIP.
- if (BasicBlock::iterator(CI) != IP || BIP == IP) {
+ if (BasicBlock::iterator(CI) != IP || BIP == &*IP) {
// Create a new cast, and leave the old cast in place in case
// it is being used as an insert point.
Ret = CastInst::Create(Op, V, Ty, "", &*IP);
@@ -84,7 +89,7 @@
// We assert at the end of the function since IP might point to an
// instruction with different dominance properties than a cast
// (an invoke for example) and not dominate BIP (but the cast does).
- assert(SE.DT.dominates(Ret, &*BIP));
+ assert(SE.DT.dominates(Ret, BIP));
rememberInstruction(Ret);
return Ret;
Index: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
+++ llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
@@ -1436,7 +1436,11 @@
// 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));
+ WidePhi = dyn_cast<PHINode>(Rewriter.expandCodeFor(AddRec, WideType, InsertPt));
+ // If the wide phi is not a phi node, for example a cast node, like bitcast,
+ // inttoptr, ptrtoint, just skip for now.
+ if (!WidePhi)
+ return nullptr;
// Remembering the WideIV increment generated by SCEVExpander allows
// widenIVUse to reuse it when widening the narrow IV's increment. We don't
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D80975.267784.patch
Type: text/x-patch
Size: 3810 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200602/a1dc4524/attachment.bin>
More information about the llvm-commits
mailing list