From: Alina Sbirlea Date: Wed, 25 Sep 2019 23:24:39 +0000 (+0000) Subject: [MemorySSA] Avoid adding Phis in the presence of unreachable blocks. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=88de695493c8df761fe83e49b6059914ed39b2c1;p=llvm [MemorySSA] Avoid adding Phis in the presence of unreachable blocks. 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 --- diff --git a/lib/Analysis/MemorySSAUpdater.cpp b/lib/Analysis/MemorySSAUpdater.cpp index bec811d17e2..b62cf08eb4c 100644 --- a/lib/Analysis/MemorySSAUpdater.cpp +++ b/lib/Analysis/MemorySSAUpdater.cpp @@ -71,11 +71,19 @@ MemoryAccess *MemorySSAUpdater::getPreviousDefRecursive( // 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 @@ -84,7 +92,9 @@ MemoryAccess *MemorySSAUpdater::getPreviousDefRecursive( // 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); @@ -315,8 +325,8 @@ void MemorySSAUpdater::insertDef(MemoryDef *MD, bool RenameUses) { SmallVector FixupList(InsertedPHIs.begin(), InsertedPHIs.end()); - SmallPtrSet 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 @@ -335,49 +345,51 @@ void MemorySSAUpdater::insertDef(MemoryDef *MD, bool RenameUses) { auto Iter = MD->getDefsIterator(); ++Iter; auto IterEnd = MSSA->getBlockDefs(MD->getBlock())->end(); - if (Iter == IterEnd) + if (Iter == IterEnd) { + SmallPtrSet DefiningBlocks; DefiningBlocks.insert(MD->getBlock()); + for (const auto &VH : InsertedPHIs) + if (const auto *RealPHI = cast_or_null(VH)) + DefiningBlocks.insert(RealPHI->getBlock()); + ForwardIDFCalculator IDFs(*MSSA->DT); + SmallVector IDFBlocks; + IDFs.setDefiningBlocks(DefiningBlocks); + IDFs.calculate(IDFBlocks); + SmallVector, 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> CachedPreviousDef; + MPhi->addIncoming(getPreviousDefFromEnd(Pred, CachedPreviousDef), + Pred); + } + } - FixupList.push_back(MD); - } - - ForwardIDFCalculator IDFs(*MSSA->DT); - SmallVector IDFBlocks; - for (const auto &VH : InsertedPHIs) - if (const auto *RealPHI = cast_or_null(VH)) - DefiningBlocks.insert(RealPHI->getBlock()); - IDFs.setDefiningBlocks(DefiningBlocks); - IDFs.calculate(IDFBlocks); - SmallVector, 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> 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(); diff --git a/test/Analysis/MemorySSA/pr40754.ll b/test/Analysis/MemorySSA/pr40754.ll index 8db320d8023..3262a0cdd46 100644 --- a/test/Analysis/MemorySSA/pr40754.ll +++ b/test/Analysis/MemorySSA/pr40754.ll @@ -11,44 +11,45 @@ target triple = "systemz-unknown" ; 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 -;