Summary:
If a block has all incoming values with the same MemoryAccess (ignoring
incoming values from unreachable blocks), then use that incoming
MemoryAccess and do not create a Phi in the first place.
Revert IDF work-around added in rL372673; it should not be required unless
the Def inserted is the first in its block.
The patch also cleans up a series of tests, added during the many
iterations on insertDef.
The patch also fixes PR43438.
The same issue that occurs in insertDef with "adding phis, hence the IDF of
Phis is needed", can also occur in fixupDefs: the `getPreviousRecursive`
call only adds Phis walking on the predecessor edges, which means there
may be the case of a Phi added walking the CFG "backwards" which
triggers the needs for an additional Phi in successor blocks.
Such Phis are added during fixupDefs only in the presence of unreachable
blocks.
Hence this highlights the need to avoid adding Phis in blocks with
unreachable predecessors in the first place.
Reviewers: george.burgess.iv
Subscribers: Prazek, sanjoy.google, llvm-commits
Tags: #llvm
Differential Revision: https://reviews.llvm.org/D67995
git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@372932
91177308-0d34-0410-b5e6-
96231b3b80d8
// Recurse to get the values in our predecessors for placement of a
// potential phi node. This will insert phi nodes if we cycle in order to
// break the cycle and have an operand.
- for (auto *Pred : predecessors(BB))
- if (MSSA->DT->isReachableFromEntry(Pred))
- PhiOps.push_back(getPreviousDefFromEnd(Pred, CachedPreviousDef));
- else
+ bool UniqueIncomingAccess = true;
+ MemoryAccess *SingleAccess = nullptr;
+ for (auto *Pred : predecessors(BB)) {
+ if (MSSA->DT->isReachableFromEntry(Pred)) {
+ auto *IncomingAccess = getPreviousDefFromEnd(Pred, CachedPreviousDef);
+ if (!SingleAccess)
+ SingleAccess = IncomingAccess;
+ else if (IncomingAccess != SingleAccess)
+ UniqueIncomingAccess = false;
+ PhiOps.push_back(IncomingAccess);
+ } else
PhiOps.push_back(MSSA->getLiveOnEntryDef());
+ }
// Now try to simplify the ops to avoid placing a phi.
// This may return null if we never created a phi yet, that's okay
// See if we can avoid the phi by simplifying it.
auto *Result = tryRemoveTrivialPhi(Phi, PhiOps);
// If we couldn't simplify, we may have to create a phi
- if (Result == Phi) {
+ if (Result == Phi && UniqueIncomingAccess && SingleAccess)
+ Result = SingleAccess;
+ else if (Result == Phi && !(UniqueIncomingAccess && SingleAccess)) {
if (!Phi)
Phi = MSSA->createMemoryPhi(BB);
SmallVector<WeakVH, 8> FixupList(InsertedPHIs.begin(), InsertedPHIs.end());
- SmallPtrSet<BasicBlock *, 2> DefiningBlocks;
-
+ // Remember the index where we may insert new phis.
+ unsigned NewPhiIndex = InsertedPHIs.size();
if (!DefBeforeSameBlock) {
// If there was a local def before us, we must have the same effect it
// did. Because every may-def is the same, any phis/etc we would create, it
auto Iter = MD->getDefsIterator();
++Iter;
auto IterEnd = MSSA->getBlockDefs(MD->getBlock())->end();
- if (Iter == IterEnd)
+ if (Iter == IterEnd) {
+ SmallPtrSet<BasicBlock *, 2> DefiningBlocks;
DefiningBlocks.insert(MD->getBlock());
+ for (const auto &VH : InsertedPHIs)
+ if (const auto *RealPHI = cast_or_null<MemoryPhi>(VH))
+ DefiningBlocks.insert(RealPHI->getBlock());
+ ForwardIDFCalculator IDFs(*MSSA->DT);
+ SmallVector<BasicBlock *, 32> IDFBlocks;
+ IDFs.setDefiningBlocks(DefiningBlocks);
+ IDFs.calculate(IDFBlocks);
+ SmallVector<AssertingVH<MemoryPhi>, 4> NewInsertedPHIs;
+ for (auto *BBIDF : IDFBlocks) {
+ auto *MPhi = MSSA->getMemoryAccess(BBIDF);
+ if (!MPhi) {
+ MPhi = MSSA->createMemoryPhi(BBIDF);
+ NewInsertedPHIs.push_back(MPhi);
+ }
+ // Add the phis created into the IDF blocks to NonOptPhis, so they are
+ // not optimized out as trivial by the call to getPreviousDefFromEnd
+ // below. Once they are complete, all these Phis are added to the
+ // FixupList, and removed from NonOptPhis inside fixupDefs(). Existing
+ // Phis in IDF may need fixing as well, and potentially be trivial
+ // before this insertion, hence add all IDF Phis. See PR43044.
+ NonOptPhis.insert(MPhi);
+ }
+ for (auto &MPhi : NewInsertedPHIs) {
+ auto *BBIDF = MPhi->getBlock();
+ for (auto *Pred : predecessors(BBIDF)) {
+ DenseMap<BasicBlock *, TrackingVH<MemoryAccess>> CachedPreviousDef;
+ MPhi->addIncoming(getPreviousDefFromEnd(Pred, CachedPreviousDef),
+ Pred);
+ }
+ }
- FixupList.push_back(MD);
- }
-
- ForwardIDFCalculator IDFs(*MSSA->DT);
- SmallVector<BasicBlock *, 32> IDFBlocks;
- for (const auto &VH : InsertedPHIs)
- if (const auto *RealPHI = cast_or_null<MemoryPhi>(VH))
- DefiningBlocks.insert(RealPHI->getBlock());
- IDFs.setDefiningBlocks(DefiningBlocks);
- IDFs.calculate(IDFBlocks);
- SmallVector<AssertingVH<MemoryPhi>, 4> NewInsertedPHIs;
- for (auto *BBIDF : IDFBlocks) {
- auto *MPhi = MSSA->getMemoryAccess(BBIDF);
- if (!MPhi) {
- MPhi = MSSA->createMemoryPhi(BBIDF);
- NewInsertedPHIs.push_back(MPhi);
- }
- // Add the phis created into the IDF blocks to NonOptPhis, so they are not
- // optimized out as trivial by the call to getPreviousDefFromEnd below. Once
- // they are complete, all these Phis are added to the FixupList, and removed
- // from NonOptPhis inside fixupDefs(). Existing Phis in IDF may need fixing
- // as well, and potentially be trivial before this insertion, hence add all
- // IDF Phis. See PR43044.
- NonOptPhis.insert(MPhi);
- }
-
- for (auto &MPhi : NewInsertedPHIs) {
- auto *BBIDF = MPhi->getBlock();
- for (auto *Pred : predecessors(BBIDF)) {
- DenseMap<BasicBlock *, TrackingVH<MemoryAccess>> CachedPreviousDef;
- MPhi->addIncoming(getPreviousDefFromEnd(Pred, CachedPreviousDef), Pred);
+ // Re-take the index where we're adding the new phis, because the above
+ // call to getPreviousDefFromEnd, may have inserted into InsertedPHIs.
+ NewPhiIndex = InsertedPHIs.size();
+ for (auto &MPhi : NewInsertedPHIs) {
+ InsertedPHIs.push_back(&*MPhi);
+ FixupList.push_back(&*MPhi);
+ }
}
+ FixupList.push_back(MD);
}
- // Remember the index where we may insert new phis.
- unsigned NewPhiIndex = InsertedPHIs.size();
- for (auto &MPhi : NewInsertedPHIs) {
- InsertedPHIs.push_back(&*MPhi);
- FixupList.push_back(&*MPhi);
- }
// Remember the index where we stopped inserting new phis above, since the
// fixupDefs call in the loop below may insert more, that are already minimal.
unsigned NewPhiIndexEnd = InsertedPHIs.size();
; Function Attrs: norecurse noreturn nounwind
define dso_local void @func_65() local_unnamed_addr {
; CHECK-LABEL: @func_65()
- br label %1
+label0:
+ br label %label1
-; <label>:1: ; preds = %.thread, %0
- br label %2
+label1: ; preds = %.thread, %label0
+ br label %label2
-; <label>:2: ; preds = %.critedge, %1
- br label %3
+label2: ; preds = %.critedge, %label1
+ br label %label3
-; <label>:3: ; preds = %5, %2
- %storemerge = phi i32 [ 0, %2 ], [ %6, %5 ]
+label3: ; preds = %label5, %label2
+ %storemerge = phi i32 [ 0, %label2 ], [ %tmp6, %label5 ]
store i32 %storemerge, i32* @g_185, align 4
- %4 = icmp ult i32 %storemerge, 2
- br i1 %4, label %5, label %.thread.loopexit
-
-; <label>:5: ; preds = %3
- %6 = add i32 %storemerge, 1
- %7 = zext i32 %6 to i64
- %8 = getelementptr [8 x [4 x [6 x i32]]], [8 x [4 x [6 x i32]]]* @g_120, i64 0, i64 undef, i64 %7, i64 undef
- %9 = load i32, i32* %8, align 4
- %10 = icmp eq i32 %9, 0
- br i1 %10, label %3, label %11
-
-; <label>:11: ; preds = %5
- %storemerge.lcssa4 = phi i32 [ %storemerge, %5 ]
- %12 = icmp eq i32 %storemerge.lcssa4, 0
- br i1 %12, label %.critedge, label %.thread.loopexit3
-
-.critedge: ; preds = %11
+ %tmp4 = icmp ult i32 %storemerge, 2
+ br i1 %tmp4, label %label5, label %.thread.loopexit
+
+label5: ; preds = %label3
+ %tmp6 = add i32 %storemerge, 1
+ %tmp7 = zext i32 %tmp6 to i64
+ %tmp8 = getelementptr [8 x [4 x [6 x i32]]], [8 x [4 x [6 x i32]]]* @g_120, i64 0, i64 undef, i64 %tmp7, i64 undef
+ %tmp9 = load i32, i32* %tmp8, align 4
+ %tmp10 = icmp eq i32 %tmp9, 0
+ br i1 %tmp10, label %label3, label %label11
+
+label11: ; preds = %label5
+ %storemerge.lcssa4 = phi i32 [ %storemerge, %label5 ]
+ %tmp12 = icmp eq i32 %storemerge.lcssa4, 0
+ br i1 %tmp12, label %.critedge, label %.thread.loopexit3
+
+.critedge: ; preds = %label11
store i16 0, i16* @g_329, align 2
- br label %2
+ br label %label2
-.thread.loopexit: ; preds = %3
+.thread.loopexit: ; preds = %label3
br label %.thread
-.thread.loopexit3: ; preds = %11
+.thread.loopexit3: ; preds = %label11
br label %.thread
.thread: ; preds = %.thread.loopexit3, %.thread.loopexit
- br label %1
+ br label %label1
}
-; RUN: opt -S -licm -enable-mssa-loop-dependency %s | FileCheck %s
-; REQUIRES: asserts
+; RUN: opt -disable-output -licm -print-memoryssa -enable-mssa-loop-dependency=true < %s 2>&1 | FileCheck %s
target datalayout = "E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-v128:64-a:8:16-n32:64"
target triple = "s390x-ibm-linux"
@g_1087 = external dso_local global i32**, align 8
; CHECK-LABEL: @f1()
+; CHECK: 5 = MemoryPhi(
+; CHECK-NOT: 7 = MemoryPhi(
define dso_local fastcc void @f1() unnamed_addr #0 {
label0:
br i1 undef, label %thread-pre-split.i.preheader, label %label5
-; RUN: opt -S -licm -enable-mssa-loop-dependency=true < %s | FileCheck %s
-; REQUIRES: asserts
+; RUN: opt -disable-output -licm -print-memoryssa -enable-mssa-loop-dependency=true < %s 2>&1 | FileCheck %s
@v_274 = external dso_local global i64, align 1
@v_295 = external dso_local global i16, align 1
@v_335 = external dso_local global i32, align 1
; CHECK-LABEL: @main()
+; CHECK-NOT: 5 = MemoryPhi(
+; CHECK-NOT: 6 = MemoryPhi(
+; CHECK: 4 = MemoryPhi(
+; CHECK-NOT: 7 = MemoryPhi(
define dso_local void @main() {
entry:
store i32 undef, i32* @v_335, align 1
--- /dev/null
+; RUN: opt -disable-output -licm -print-memoryssa -enable-mssa-loop-dependency=true < %s 2>&1 | FileCheck %s
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK-LABEL: @main()
+; CHECK: 5 = MemoryPhi(
+; CHECK-NOT: 7 = MemoryPhi(
+@v_67 = external dso_local global i32, align 1
+@v_76 = external dso_local global i16, align 1
+@v_86 = external dso_local global i16 *, align 1
+
+define dso_local void @main() {
+entry:
+ %v_59 = alloca i16, align 2
+ br label %for.cond
+
+for.cond: ; preds = %for.body, %entry
+ br i1 undef, label %for.body, label %for.end
+
+for.body: ; preds = %for.cond
+ store i16 undef, i16* %v_59, align 2
+ br label %for.cond
+
+for.end: ; preds = %for.cond
+ br i1 undef, label %if.else568, label %cond.end82
+
+cond.false69: ; No predecessors!
+ br label %cond.end82
+
+cond.end82: ; preds = %cond.false69, %cond.true55
+ br i1 undef, label %if.else568, label %land.lhs.true87
+
+land.lhs.true87: ; preds = %cond.end82
+ br i1 undef, label %if.then88, label %if.else568
+
+if.then88: ; preds = %land.lhs.true87
+ store i16 * @v_76, i16 ** @v_86, align 1
+ br label %if.end569
+
+if.else568: ; preds = %land.lhs.true87, %cond.end82, %for.end
+ store volatile i32 undef, i32 * @v_67, align 1
+ br label %if.end569
+
+if.end569: ; preds = %if.else568, %if.then88
+ ret void
+}
+
projects / llvm / commitdiff