Index: llvm/trunk/include/llvm/Transforms/Utils/MemorySSA.h =================================================================== --- llvm/trunk/include/llvm/Transforms/Utils/MemorySSA.h +++ llvm/trunk/include/llvm/Transforms/Utils/MemorySSA.h @@ -73,7 +73,6 @@ #define LLVM_TRANSFORMS_UTILS_MEMORYSSA_H #include "llvm/ADT/DenseMap.h" -#include "llvm/ADT/DenseSet.h" #include "llvm/ADT/GraphTraits.h" #include "llvm/ADT/SmallPtrSet.h" #include "llvm/ADT/SmallVector.h" @@ -110,7 +109,6 @@ class LLVMContext; class raw_ostream; namespace MSSAHelpers { - struct AllAccessTag {}; struct DefsOnlyTag {}; } @@ -204,6 +202,10 @@ friend class MemoryDef; friend class MemoryPhi; + /// \brief Used by MemorySSA to change the block of a MemoryAccess when it is + /// moved. + void setBlock(BasicBlock *BB) { Block = BB; } + /// \brief Used for debugging and tracking things about MemoryAccesses. /// Guaranteed unique among MemoryAccesses, no guarantees otherwise. virtual unsigned getID() const = 0; @@ -248,7 +250,7 @@ protected: friend class MemorySSA; - + friend class MemorySSAUpdater; MemoryUseOrDef(LLVMContext &C, MemoryAccess *DMA, unsigned Vty, Instruction *MI, BasicBlock *BB) : MemoryAccess(C, Vty, BB, 1), MemoryInst(MI) { @@ -626,8 +628,8 @@ /// Returns the new MemoryAccess. /// This should be called when a memory instruction is created that is being /// used to replace an existing memory instruction. It will *not* create PHI - /// nodes, or verify the clobbering definition. The clobbering definition - /// must be non-null. + /// nodes, or verify the clobbering definition. + /// /// Note: If a MemoryAccess already exists for I, this function will make it /// inaccessible and it *must* have removeMemoryAccess called on it. MemoryUseOrDef *createMemoryAccessBefore(Instruction *I, @@ -637,15 +639,6 @@ MemoryAccess *Definition, MemoryAccess *InsertPt); - // \brief Splice \p What to just before \p Where. - // - // In order to be efficient, the following conditions must be met: - // - \p Where dominates \p What, - // - All memory accesses in [\p Where, \p What) are no-alias with \p What. - // - // TODO: relax the MemoryDef requirement on Where. - void spliceMemoryAccessAbove(MemoryDef *Where, MemoryUseOrDef *What); - /// \brief Remove a MemoryAccess from MemorySSA, including updating all /// definitions and uses. /// This should be called when a memory instruction that has a MemoryAccess @@ -674,6 +667,7 @@ // Used by Memory SSA annotater, dumpers, and wrapper pass friend class MemorySSAAnnotatedWriter; friend class MemorySSAPrinterLegacyPass; + friend class MemorySSAUpdater; void verifyDefUses(Function &F) const; void verifyDomination(Function &F) const; @@ -691,6 +685,11 @@ return It == PerBlockDefs.end() ? nullptr : It->second.get(); } + // This is used by the updater to perform the internal memoryssa machinations + // for moves. It does not always leave the IR in a correct state, and relies + // on the updater to fixup what it breaks, so it is not public. + void moveTo(MemoryUseOrDef *What, BasicBlock *BB, AccessList::iterator Where); + private: class CachingWalker; class OptimizeUses; @@ -712,6 +711,7 @@ MemoryUseOrDef *createDefinedAccess(Instruction *, MemoryAccess *); MemoryAccess *findDominatingDef(BasicBlock *, enum InsertionPlace); void removeFromLookups(MemoryAccess *); + void removeFromLists(MemoryAccess *, bool ShouldDelete = true); void placePHINodes(const SmallPtrSetImpl &, const DenseMap &); @@ -752,6 +752,51 @@ unsigned NextID; }; +// An automatic updater for MemorySSA that handles arbitrary insertion, +// deletion, and moves. It performs phi insertion where necessary, and +// automatically updates the MemorySSA IR to be correct. +// While updating loads or removing instructions is often easy enough to not +// need this, updating stores should generally not be attemped outside this +// API. +// +// Basic API usage: +// Create the memory access you want for the instruction (this is mainly so +// we know where it is, without having to duplicate the entire set of create +// functions MemorySSA supports). +// Call insertDef or insertUse depending on whether it's a MemoryUse or a +// MemoryDef. +// That's it. +// +// For moving, first, move the instruction itself using the normal SSA +// instruction moving API, then just call moveBefore or moveAfter with the right +// arguments. +// +class MemorySSAUpdater { +private: + MemorySSA *MSSA; + SmallVector InsertedPHIs; + SmallPtrSet VisitedBlocks; + +public: + MemorySSAUpdater(MemorySSA *MSSA) : MSSA(MSSA) {} + void insertDef(MemoryDef *Def); + void insertUse(MemoryUse *Use); + void moveBefore(MemoryUseOrDef *What, MemoryUseOrDef *Where); + void moveAfter(MemoryUseOrDef *What, MemoryUseOrDef *Where); + +private: + void moveTo(MemoryUseOrDef *What, BasicBlock *BB, + MemorySSA::AccessList::iterator Where); + MemoryAccess *getPreviousDef(MemoryAccess *); + MemoryAccess *getPreviousDefInBlock(MemoryAccess *); + MemoryAccess *getPreviousDefFromEnd(BasicBlock *); + MemoryAccess *getPreviousDefRecursive(BasicBlock *); + MemoryAccess *recursePhi(MemoryAccess *Phi); + template + MemoryAccess *tryRemoveTrivialPhi(MemoryPhi *Phi, RangeType &Operands); + void fixupDefs(const SmallVectorImpl &); +}; + // This pass does eager building and then printing of MemorySSA. It is used by // the tests to be able to build, dump, and verify Memory SSA. class MemorySSAPrinterLegacyPass : public FunctionPass { Index: llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp =================================================================== --- llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp +++ llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp @@ -1626,6 +1626,18 @@ } } +// Move What before Where in the IR. The end result is taht What will belong to +// the right lists and have the right Block set, but will not otherwise be +// correct. It will not have the right defining access, and if it is a def, +// things below it will not properly be updated. +void MemorySSA::moveTo(MemoryUseOrDef *What, BasicBlock *BB, + AccessList::iterator Where) { + // Keep it in the lookup tables, remove from the lists + removeFromLists(What, false); + What->setBlock(BB); + insertIntoListsBefore(What, BB, Where); +} + MemoryPhi *MemorySSA::createMemoryPhi(BasicBlock *BB) { assert(!getMemoryAccess(BB) && "MemoryPhi already exists for this BB"); MemoryPhi *Phi = new MemoryPhi(BB->getContext(), BB, NextID++); @@ -1681,29 +1693,6 @@ return NewAccess; } -void MemorySSA::spliceMemoryAccessAbove(MemoryDef *Where, - MemoryUseOrDef *What) { - assert(What != getLiveOnEntryDef() && Where != getLiveOnEntryDef() && - "Can't splice (above) LOE."); - assert(dominates(Where, What) && "Only upwards splices are permitted."); - - if (Where == What) - return; - if (isa(What)) { - // TODO: possibly use removeMemoryAccess' more efficient RAUW - What->replaceAllUsesWith(What->getDefiningAccess()); - What->setDefiningAccess(Where->getDefiningAccess()); - Where->setDefiningAccess(What); - } - AccessList *Src = getWritableBlockAccesses(What->getBlock()); - AccessList *Dest = getWritableBlockAccesses(Where->getBlock()); - Dest->splice(AccessList::iterator(Where), *Src, What); - - BlockNumberingValid.erase(What->getBlock()); - if (What->getBlock() != Where->getBlock()) - BlockNumberingValid.erase(Where->getBlock()); -} - /// \brief Helper function to create new memory accesses MemoryUseOrDef *MemorySSA::createNewAccess(Instruction *I) { // The assume intrinsic has a control dependency which we model by claiming @@ -1795,9 +1784,6 @@ } /// \brief Properly remove \p MA from all of MemorySSA's lookup tables. -/// -/// Because of the way the intrusive list and use lists work, it is important to -/// do removal in the right order. void MemorySSA::removeFromLookups(MemoryAccess *MA) { assert(MA->use_empty() && "Trying to remove memory access that still has uses"); @@ -1818,7 +1804,15 @@ auto VMA = ValueToMemoryAccess.find(MemoryInst); if (VMA->second == MA) ValueToMemoryAccess.erase(VMA); +} +/// \brief Properly remove \p MA from all of MemorySSA's lists. +/// +/// Because of the way the intrusive list and use lists work, it is important to +/// do removal in the right order. +/// ShouldDelete defaults to true, and will cause the memory access to also be +/// deleted, not just removed. +void MemorySSA::removeFromLists(MemoryAccess *MA, bool ShouldDelete) { // The access list owns the reference, so we erase it from the non-owning list // first. if (!isa(MA)) { @@ -1829,9 +1823,15 @@ PerBlockDefs.erase(DefsIt); } + // The erase call here will delete it. If we don't want it deleted, we call + // remove instead. auto AccessIt = PerBlockAccesses.find(MA->getBlock()); std::unique_ptr &Accesses = AccessIt->second; - Accesses->erase(MA); + if (ShouldDelete) + Accesses->erase(MA); + else + Accesses->remove(MA); + if (Accesses->empty()) PerBlockAccesses.erase(AccessIt); } @@ -1855,7 +1855,7 @@ } // Re-point the uses at our defining access - if (!MA->use_empty()) { + if (!isa(MA) && !MA->use_empty()) { // Reset optimized on users of this store, and reset the uses. // A few notes: // 1. This is a slightly modified version of RAUW to avoid walking the @@ -1880,6 +1880,7 @@ // The call below to erase will destroy MA, so we can't change the order we // are doing things here removeFromLookups(MA); + removeFromLists(MA); } void MemorySSA::print(raw_ostream &OS) const { @@ -2396,4 +2397,343 @@ return Use->getDefiningAccess(); return StartingAccess; } +// This is the marker algorithm from "Simple and Efficient Construction of +// Static Single Assignment Form" +// The simple, non-marker algorithm places phi nodes at any join +// Here, we place markers, and only place phi nodes if they end up necessary. +// They are only necessary if they break a cycle (IE we recursively visit +// ourselves again), or we discover, while getting the value of the operands, +// that there are two or more definitions needing to be merged. +// This still will leave non-minimal form in the case of irreducible control +// flow, where phi nodes may be in cycles with themselves, but unnecessary. +MemoryAccess *MemorySSAUpdater::getPreviousDefRecursive(BasicBlock *BB) { + // Single predecessor case, just recurse, we can only have one definition. + if (BasicBlock *Pred = BB->getSinglePredecessor()) { + return getPreviousDefFromEnd(Pred); + } else if (VisitedBlocks.count(BB)) { + // We hit our node again, meaning we had a cycle, we must insert a phi + // node to break it so we have an operand. The only case this will + // insert useless phis is if we have irreducible control flow. + return MSSA->createMemoryPhi(BB); + } else if (VisitedBlocks.insert(BB).second) { + // Mark us visited so we can detect a cycle + SmallVector PhiOps; + + // 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)) + PhiOps.push_back(getPreviousDefFromEnd(Pred)); + + // 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 + MemoryPhi *Phi = dyn_cast_or_null(MSSA->getMemoryAccess(BB)); + bool PHIExistsButNeedsUpdate = false; + // See if the existing phi operands match what we need. + // Unlike normal SSA, we only allow one phi node per block, so we can't just + // create a new one. + if (Phi && Phi->getNumOperands() != 0) + if (!std::equal(Phi->op_begin(), Phi->op_end(), PhiOps.begin())) { + PHIExistsButNeedsUpdate = true; + } + + // 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 (!Phi) + Phi = MSSA->createMemoryPhi(BB); + + // These will have been filled in by the recursive read we did above. + if (PHIExistsButNeedsUpdate) { + std::copy(PhiOps.begin(), PhiOps.end(), Phi->op_begin()); + std::copy(pred_begin(BB), pred_end(BB), Phi->block_begin()); + } else { + unsigned i = 0; + for (auto *Pred : predecessors(BB)) + Phi->addIncoming(PhiOps[i++], Pred); + } + + Result = Phi; + } + if (MemoryPhi *MP = dyn_cast(Result)) + InsertedPHIs.push_back(MP); + // Set ourselves up for the next variable by resetting visited state. + VisitedBlocks.erase(BB); + return Result; + } + llvm_unreachable("Should have hit one of the three cases above"); +} + +// This starts at the memory access, and goes backwards in the block to find the +// previous definition. If a definition is not found the block of the access, +// it continues globally, creating phi nodes to ensure we have a single +// definition. +MemoryAccess *MemorySSAUpdater::getPreviousDef(MemoryAccess *MA) { + auto *LocalResult = getPreviousDefInBlock(MA); + + return LocalResult ? LocalResult : getPreviousDefRecursive(MA->getBlock()); +} + +// This starts at the memory access, and goes backwards in the block to the find +// the previous definition. If the definition is not found in the block of the +// access, it returns nullptr. +MemoryAccess *MemorySSAUpdater::getPreviousDefInBlock(MemoryAccess *MA) { + auto *Defs = MSSA->getWritableBlockDefs(MA->getBlock()); + + // It's possible there are no defs, or we got handed the first def to start. + if (Defs) { + // If this is a def, we can just use the def iterators. + if (!isa(MA)) { + auto Iter = MA->getReverseDefsIterator(); + ++Iter; + if (Iter != Defs->rend()) + return &*Iter; + } else { + // Otherwise, have to walk the all access iterator. + auto Iter = MA->getReverseIterator(); + ++Iter; + while (&*Iter != &*Defs->begin()) { + if (!isa(*Iter)) + return &*Iter; + --Iter; + } + // At this point it must be pointing at firstdef + assert(&*Iter == &*Defs->begin() && + "Should have hit first def walking backwards"); + return &*Iter; + } + } + return nullptr; +} + +// This starts at the end of block +MemoryAccess *MemorySSAUpdater::getPreviousDefFromEnd(BasicBlock *BB) { + auto *Defs = MSSA->getWritableBlockDefs(BB); + + if (Defs) + return &*Defs->rbegin(); + + return getPreviousDefRecursive(BB); +} +// Recurse over a set of phi uses to eliminate the trivial ones +MemoryAccess *MemorySSAUpdater::recursePhi(MemoryAccess *Phi) { + if (!Phi) + return nullptr; + TrackingVH Res(Phi); + SmallVector, 8> Uses; + std::copy(Phi->user_begin(), Phi->user_end(), std::back_inserter(Uses)); + for (auto &U : Uses) { + if (MemoryPhi *UsePhi = dyn_cast(&*U)) { + auto OperRange = UsePhi->operands(); + tryRemoveTrivialPhi(UsePhi, OperRange); + } + } + return Res; +} + +// Eliminate trivial phis +// Phis are trivial if they are defined either by themselves, or all the same +// argument. +// IE phi(a, a) or b = phi(a, b) or c = phi(a, a, c) +// We recursively try to remove them. +template +MemoryAccess *MemorySSAUpdater::tryRemoveTrivialPhi(MemoryPhi *Phi, + RangeType &Operands) { + // Detect equal or self arguments + MemoryAccess *Same = nullptr; + for (auto &Op : Operands) { + // If the same or self, good so far + if (Op == Phi || Op == Same) + continue; + // not the same, return the phi since it's not eliminatable by us + if (Same) + return Phi; + Same = cast(Op); + } + // Never found a non-self reference, the phi is undef + if (Same == nullptr) + return MSSA->getLiveOnEntryDef(); + if (Phi) { + Phi->replaceAllUsesWith(Same); + MSSA->removeMemoryAccess(Phi); + } + + // We should only end up recursing in case we replaced something, in which + // case, we may have made other Phis trivial. + return recursePhi(Same); +} + +void MemorySSAUpdater::insertUse(MemoryUse *MU) { + InsertedPHIs.clear(); + MU->setDefiningAccess(getPreviousDef(MU)); + // Unlike for defs, there is no extra work to do. Because uses do not create + // new may-defs, there are only two cases: + // + // 1. There was a def already below us, and therefore, we should not have + // created a phi node because it was already needed for the def. + // + // 2. There is no def below us, and therefore, there is no extra renaming work + // to do. +} + +void setMemoryPhiValueForBlock(MemoryPhi *MP, const BasicBlock *BB, + MemoryAccess *NewDef) { + // Replace any operand with us an incoming block with the new defining + // access. + int i = MP->getBasicBlockIndex(BB); + assert(i != -1 && "Should have found the basic block in the phi"); + while (MP->getIncomingBlock(i) == BB) { + // Unlike above, there is already a phi node here, so we only need + // to set the right value. + MP->setIncomingValue(i, NewDef); + ++i; + } +} + +// A brief description of the algorithm: +// First, we compute what should define the new def, using the SSA +// construction algorithm. +// Then, we update the defs below us (and any new phi nodes) in the graph to +// point to the correct new defs, to ensure we only have one variable, and no +// disconnected stores. +void MemorySSAUpdater::insertDef(MemoryDef *MD) { + InsertedPHIs.clear(); + + // See if we had a local def, and if not, go hunting. + MemoryAccess *DefBefore = getPreviousDefInBlock(MD); + bool DefBeforeSameBlock = DefBefore != nullptr; + if (!DefBefore) + DefBefore = getPreviousDefRecursive(MD->getBlock()); + + // There is a def before us, which means we can replace any store/phi uses + // of that thing with us, since we are in the way of whatever was there + // before. + // We now define that def's memorydefs and memoryphis + for (auto UI = DefBefore->use_begin(), UE = DefBefore->use_end(); UI != UE;) { + Use &U = *UI++; + // Leave the uses alone + if (isa(U.getUser())) + continue; + U.set(MD); + } + // and that def is now our defining access. + // We change them in this order otherwise we will appear in the use list + // above and reset ourselves. + MD->setDefiningAccess(DefBefore); + + SmallVector FixupList(InsertedPHIs.begin(), + InsertedPHIs.end()); + 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 + // would also have created. If there was no local def before us, we + // performed a global update, and have to search all successors and make + // sure we update the first def in each of them (following all paths until + // we hit the first def along each path). This may also insert phi nodes. + // TODO: There are other cases we can skip this work, such as when we have a + // single successor, and only used a straight line of single pred blocks + // backwards to find the def. To make that work, we'd have to track whether + // getDefRecursive only ever used the single predecessor case. These types + // of paths also only exist in between CFG simplifications. + FixupList.push_back(MD); + } + + while (!FixupList.empty()) { + unsigned StartingPHISize = InsertedPHIs.size(); + fixupDefs(FixupList); + FixupList.clear(); + // Put any new phis on the fixup list, and process them + FixupList.append(InsertedPHIs.end() - StartingPHISize, InsertedPHIs.end()); + } +} + +void MemorySSAUpdater::fixupDefs(const SmallVectorImpl &Vars) { + SmallPtrSet Seen; + SmallVector Worklist; + for (auto *NewDef : Vars) { + // First, see if there is a local def after the operand. + auto *Defs = MSSA->getWritableBlockDefs(NewDef->getBlock()); + auto DefIter = NewDef->getDefsIterator(); + + // If there is a local def after us, we only have to rename that. + if (++DefIter != Defs->end()) { + cast(DefIter)->setDefiningAccess(NewDef); + continue; + } + + // Otherwise, we need to search down through the CFG. + // For each of our successors, handle it directly if their is a phi, or + // place on the fixup worklist. + for (const auto *S : successors(NewDef->getBlock())) { + if (auto *MP = MSSA->getMemoryAccess(S)) + setMemoryPhiValueForBlock(MP, NewDef->getBlock(), NewDef); + else + Worklist.push_back(S); + } + + while (!Worklist.empty()) { + const BasicBlock *FixupBlock = Worklist.back(); + Worklist.pop_back(); + + // Get the first def in the block that isn't a phi node. + if (auto *Defs = MSSA->getWritableBlockDefs(FixupBlock)) { + auto *FirstDef = &*Defs->begin(); + // The loop above and below should have taken care of phi nodes + assert(!isa(FirstDef) && + "Should have already handled phi nodes!"); + // We are now this def's defining access, make sure we actually dominate + // it + assert(MSSA->dominates(NewDef, FirstDef) && + "Should have dominated the new access"); + + // This may insert new phi nodes, because we are not guaranteed the + // block we are processing has a single pred, and depending where the + // store was inserted, it may require phi nodes below it. + cast(FirstDef)->setDefiningAccess(getPreviousDef(FirstDef)); + return; + } + // We didn't find a def, so we must continue. + for (const auto *S : successors(FixupBlock)) { + // If there is a phi node, handle it. + // Otherwise, put the block on the worklist + if (auto *MP = MSSA->getMemoryAccess(S)) + setMemoryPhiValueForBlock(MP, FixupBlock, NewDef); + else { + // If we cycle, we should have ended up at a phi node that we already + // processed. FIXME: Double check this + if (!Seen.insert(S).second) + continue; + Worklist.push_back(S); + } + } + } + } +} + +// Move What before Where in the MemorySSA IR. +void MemorySSAUpdater::moveTo(MemoryUseOrDef *What, BasicBlock *BB, + MemorySSA::AccessList::iterator Where) { + // Replace all our users with our defining access. + What->replaceAllUsesWith(What->getDefiningAccess()); + + // Let MemorySSA take care of moving it around in the lists. + MSSA->moveTo(What, BB, Where); + + // Now reinsert it into the IR and do whatever fixups needed. + if (auto *MD = dyn_cast(What)) + insertDef(MD); + else + insertUse(cast(What)); +} +// Move What before Where in the MemorySSA IR. +void MemorySSAUpdater::moveBefore(MemoryUseOrDef *What, MemoryUseOrDef *Where) { + moveTo(What, Where->getBlock(), Where->getIterator()); +} + +// Move What after Where in the MemorySSA IR. +void MemorySSAUpdater::moveAfter(MemoryUseOrDef *What, MemoryUseOrDef *Where) { + moveTo(What, Where->getBlock(), ++Where->getIterator()); +} + } // namespace llvm Index: llvm/trunk/unittests/Transforms/Utils/MemorySSA.cpp =================================================================== --- llvm/trunk/unittests/Transforms/Utils/MemorySSA.cpp +++ llvm/trunk/unittests/Transforms/Utils/MemorySSA.cpp @@ -104,11 +104,145 @@ EXPECT_TRUE(isa(DefiningAccess)); MSSA.verifyMemorySSA(); } +TEST_F(MemorySSATest, CreateLoadsAndStoreUpdater) { + // We create a diamond, then build memoryssa with no memory accesses, and + // incrementally update it by inserting a store in the, entry, a load in the + // merge point, then a store in the branch, another load in the merge point, + // and then a store in the entry. + F = Function::Create( + FunctionType::get(B.getVoidTy(), {B.getInt8PtrTy()}, false), + GlobalValue::ExternalLinkage, "F", &M); + BasicBlock *Entry(BasicBlock::Create(C, "", F)); + BasicBlock *Left(BasicBlock::Create(C, "", F)); + BasicBlock *Right(BasicBlock::Create(C, "", F)); + BasicBlock *Merge(BasicBlock::Create(C, "", F)); + B.SetInsertPoint(Entry); + B.CreateCondBr(B.getTrue(), Left, Right); + B.SetInsertPoint(Left, Left->begin()); + Argument *PointerArg = &*F->arg_begin(); + B.SetInsertPoint(Left); + B.CreateBr(Merge); + B.SetInsertPoint(Right); + B.CreateBr(Merge); + + setupAnalyses(); + MemorySSA &MSSA = *Analyses->MSSA; + MemorySSAUpdater Updater(&MSSA); + // Add the store + B.SetInsertPoint(Entry, Entry->begin()); + StoreInst *EntryStore = B.CreateStore(B.getInt8(16), PointerArg); + MemoryAccess *EntryStoreAccess = MSSA.createMemoryAccessInBB( + EntryStore, nullptr, Entry, MemorySSA::Beginning); + Updater.insertDef(cast(EntryStoreAccess)); + + // Add the load + B.SetInsertPoint(Merge, Merge->begin()); + LoadInst *FirstLoad = B.CreateLoad(PointerArg); + + // MemoryPHI should not already exist. + MemoryPhi *MP = MSSA.getMemoryAccess(Merge); + EXPECT_EQ(MP, nullptr); + + // Create the load memory access + MemoryUse *FirstLoadAccess = cast(MSSA.createMemoryAccessInBB( + FirstLoad, nullptr, Merge, MemorySSA::Beginning)); + Updater.insertUse(FirstLoadAccess); + // Should just have a load using the entry access, because it should discover + // the phi is trivial + EXPECT_EQ(FirstLoadAccess->getDefiningAccess(), EntryStoreAccess); + + // Create a store on the left + // Add the store + B.SetInsertPoint(Left, Left->begin()); + StoreInst *LeftStore = B.CreateStore(B.getInt8(16), PointerArg); + MemoryAccess *LeftStoreAccess = MSSA.createMemoryAccessInBB( + LeftStore, nullptr, Left, MemorySSA::Beginning); + Updater.insertDef(cast(LeftStoreAccess)); + // We don't touch existing loads, so we need to create a new one to get a phi + // Add the second load + B.SetInsertPoint(Merge, Merge->begin()); + LoadInst *SecondLoad = B.CreateLoad(PointerArg); + + // MemoryPHI should not already exist. + MP = MSSA.getMemoryAccess(Merge); + EXPECT_EQ(MP, nullptr); + + // Create the load memory access + MemoryUse *SecondLoadAccess = cast(MSSA.createMemoryAccessInBB( + SecondLoad, nullptr, Merge, MemorySSA::Beginning)); + Updater.insertUse(SecondLoadAccess); + // Now the load should be a phi of the entry store and the left store + MemoryPhi *MergePhi = + dyn_cast(SecondLoadAccess->getDefiningAccess()); + EXPECT_NE(MergePhi, nullptr); + EXPECT_EQ(MergePhi->getIncomingValue(0), EntryStoreAccess); + EXPECT_EQ(MergePhi->getIncomingValue(1), LeftStoreAccess); + // Now create a store below the existing one in the entry + B.SetInsertPoint(Entry, --Entry->end()); + StoreInst *SecondEntryStore = B.CreateStore(B.getInt8(16), PointerArg); + MemoryAccess *SecondEntryStoreAccess = MSSA.createMemoryAccessInBB( + SecondEntryStore, nullptr, Entry, MemorySSA::End); + Updater.insertDef(cast(SecondEntryStoreAccess)); + // and make sure the phi below it got updated, despite being blocks away + MergePhi = dyn_cast(SecondLoadAccess->getDefiningAccess()); + EXPECT_NE(MergePhi, nullptr); + EXPECT_EQ(MergePhi->getIncomingValue(0), SecondEntryStoreAccess); + EXPECT_EQ(MergePhi->getIncomingValue(1), LeftStoreAccess); + MSSA.verifyMemorySSA(); +} + +TEST_F(MemorySSATest, CreateALoadUpdater) { + // We create a diamond, then build memoryssa with no memory accesses, and + // incrementally update it by inserting a store in one of the branches, and a + // load in the merge point + F = Function::Create( + FunctionType::get(B.getVoidTy(), {B.getInt8PtrTy()}, false), + GlobalValue::ExternalLinkage, "F", &M); + BasicBlock *Entry(BasicBlock::Create(C, "", F)); + BasicBlock *Left(BasicBlock::Create(C, "", F)); + BasicBlock *Right(BasicBlock::Create(C, "", F)); + BasicBlock *Merge(BasicBlock::Create(C, "", F)); + B.SetInsertPoint(Entry); + B.CreateCondBr(B.getTrue(), Left, Right); + B.SetInsertPoint(Left, Left->begin()); + Argument *PointerArg = &*F->arg_begin(); + B.SetInsertPoint(Left); + B.CreateBr(Merge); + B.SetInsertPoint(Right); + B.CreateBr(Merge); + + setupAnalyses(); + MemorySSA &MSSA = *Analyses->MSSA; + MemorySSAUpdater Updater(&MSSA); + B.SetInsertPoint(Left, Left->begin()); + // Add the store + StoreInst *SI = B.CreateStore(B.getInt8(16), PointerArg); + MemoryAccess *StoreAccess = + MSSA.createMemoryAccessInBB(SI, nullptr, Left, MemorySSA::Beginning); + Updater.insertDef(cast(StoreAccess)); + + // Add the load + B.SetInsertPoint(Merge, Merge->begin()); + LoadInst *LoadInst = B.CreateLoad(PointerArg); + + // MemoryPHI should not already exist. + MemoryPhi *MP = MSSA.getMemoryAccess(Merge); + EXPECT_EQ(MP, nullptr); + + // Create the load memory acccess + MemoryUse *LoadAccess = cast(MSSA.createMemoryAccessInBB( + LoadInst, nullptr, Merge, MemorySSA::Beginning)); + Updater.insertUse(LoadAccess); + MemoryAccess *DefiningAccess = LoadAccess->getDefiningAccess(); + EXPECT_TRUE(isa(DefiningAccess)); + MSSA.verifyMemorySSA(); +} TEST_F(MemorySSATest, MoveAStore) { // We create a diamond where there is a in the entry, a store on one side, and // a load at the end. After building MemorySSA, we test updating by moving - // the store from the side block to the entry block. + // the store from the side block to the entry block. This destroys the old + // access. F = Function::Create( FunctionType::get(B.getVoidTy(), {B.getInt8PtrTy()}, false), GlobalValue::ExternalLinkage, "F", &M); @@ -140,6 +274,96 @@ MSSA.verifyMemorySSA(); } +TEST_F(MemorySSATest, MoveAStoreUpdater) { + // We create a diamond where there is a in the entry, a store on one side, and + // a load at the end. After building MemorySSA, we test updating by moving + // the store from the side block to the entry block. This destroys the old + // access. + F = Function::Create( + FunctionType::get(B.getVoidTy(), {B.getInt8PtrTy()}, false), + GlobalValue::ExternalLinkage, "F", &M); + BasicBlock *Entry(BasicBlock::Create(C, "", F)); + BasicBlock *Left(BasicBlock::Create(C, "", F)); + BasicBlock *Right(BasicBlock::Create(C, "", F)); + BasicBlock *Merge(BasicBlock::Create(C, "", F)); + B.SetInsertPoint(Entry); + Argument *PointerArg = &*F->arg_begin(); + StoreInst *EntryStore = B.CreateStore(B.getInt8(16), PointerArg); + B.CreateCondBr(B.getTrue(), Left, Right); + B.SetInsertPoint(Left); + auto *SideStore = B.CreateStore(B.getInt8(16), PointerArg); + BranchInst::Create(Merge, Left); + BranchInst::Create(Merge, Right); + B.SetInsertPoint(Merge); + auto *MergeLoad = B.CreateLoad(PointerArg); + setupAnalyses(); + MemorySSA &MSSA = *Analyses->MSSA; + MemorySSAUpdater Updater(&MSSA); + + // Move the store + SideStore->moveBefore(Entry->getTerminator()); + auto *EntryStoreAccess = MSSA.getMemoryAccess(EntryStore); + auto *SideStoreAccess = MSSA.getMemoryAccess(SideStore); + auto *NewStoreAccess = MSSA.createMemoryAccessAfter( + SideStore, EntryStoreAccess, EntryStoreAccess); + // Before, the load will point to a phi of the EntryStore and SideStore. + auto *LoadAccess = cast(MSSA.getMemoryAccess(MergeLoad)); + EXPECT_TRUE(isa(LoadAccess->getDefiningAccess())); + MemoryPhi *MergePhi = cast(LoadAccess->getDefiningAccess()); + EXPECT_EQ(MergePhi->getIncomingValue(1), EntryStoreAccess); + EXPECT_EQ(MergePhi->getIncomingValue(0), SideStoreAccess); + MSSA.removeMemoryAccess(SideStoreAccess); + Updater.insertDef(cast(NewStoreAccess)); + // After it's a phi of the new side store access. + EXPECT_EQ(MergePhi->getIncomingValue(0), NewStoreAccess); + EXPECT_EQ(MergePhi->getIncomingValue(1), NewStoreAccess); + MSSA.verifyMemorySSA(); +} + +TEST_F(MemorySSATest, MoveAStoreUpdaterMove) { + // We create a diamond where there is a in the entry, a store on one side, and + // a load at the end. After building MemorySSA, we test updating by moving + // the store from the side block to the entry block. This does not destroy + // the old access. + F = Function::Create( + FunctionType::get(B.getVoidTy(), {B.getInt8PtrTy()}, false), + GlobalValue::ExternalLinkage, "F", &M); + BasicBlock *Entry(BasicBlock::Create(C, "", F)); + BasicBlock *Left(BasicBlock::Create(C, "", F)); + BasicBlock *Right(BasicBlock::Create(C, "", F)); + BasicBlock *Merge(BasicBlock::Create(C, "", F)); + B.SetInsertPoint(Entry); + Argument *PointerArg = &*F->arg_begin(); + StoreInst *EntryStore = B.CreateStore(B.getInt8(16), PointerArg); + B.CreateCondBr(B.getTrue(), Left, Right); + B.SetInsertPoint(Left); + auto *SideStore = B.CreateStore(B.getInt8(16), PointerArg); + BranchInst::Create(Merge, Left); + BranchInst::Create(Merge, Right); + B.SetInsertPoint(Merge); + auto *MergeLoad = B.CreateLoad(PointerArg); + setupAnalyses(); + MemorySSA &MSSA = *Analyses->MSSA; + MemorySSAUpdater Updater(&MSSA); + + // Move the store + auto *EntryStoreAccess = MSSA.getMemoryAccess(EntryStore); + auto *SideStoreAccess = MSSA.getMemoryAccess(SideStore); + // Before, the load will point to a phi of the EntryStore and SideStore. + auto *LoadAccess = cast(MSSA.getMemoryAccess(MergeLoad)); + EXPECT_TRUE(isa(LoadAccess->getDefiningAccess())); + MemoryPhi *MergePhi = cast(LoadAccess->getDefiningAccess()); + EXPECT_EQ(MergePhi->getIncomingValue(1), EntryStoreAccess); + EXPECT_EQ(MergePhi->getIncomingValue(0), SideStoreAccess); + SideStore->moveBefore(*EntryStore->getParent(), ++EntryStore->getIterator()); + Updater.moveAfter(SideStoreAccess, EntryStoreAccess); + // After, it's a phi of the side store. + EXPECT_EQ(MergePhi->getIncomingValue(0), SideStoreAccess); + EXPECT_EQ(MergePhi->getIncomingValue(1), SideStoreAccess); + + MSSA.verifyMemorySSA(); +} + TEST_F(MemorySSATest, RemoveAPhi) { // We create a diamond where there is a store on one side, and then a load // after the merge point. This enables us to test a bunch of different @@ -485,9 +709,8 @@ EXPECT_EQ(NewLoadAccess->getDefiningAccess(), LoadClobber); } -#if 0 -// Test out MemorySSA::spliceMemoryAccessAbove. -TEST_F(MemorySSATest, SpliceAboveMemoryDef) { +// Test out MemorySSAUpdater::moveBefore +TEST_F(MemorySSATest, MoveAboveMemoryDef) { F = Function::Create(FunctionType::get(B.getVoidTy(), {}, false), GlobalValue::ExternalLinkage, "F", &M); B.SetInsertPoint(BasicBlock::Create(C, "", F)); @@ -501,7 +724,6 @@ StoreInst *StoreB = B.CreateStore(ConstantInt::get(Int8, 0), B_); LoadInst *LoadB = B.CreateLoad(B_); StoreInst *StoreA1 = B.CreateStore(ConstantInt::get(Int8, 4), A); - // splice this above StoreB StoreInst *StoreC = B.CreateStore(ConstantInt::get(Int8, 4), C); StoreInst *StoreA2 = B.CreateStore(ConstantInt::get(Int8, 4), A); LoadInst *LoadC = B.CreateLoad(C); @@ -510,9 +732,10 @@ MemorySSA &MSSA = *Analyses->MSSA; MemorySSAWalker &Walker = *Analyses->Walker; + MemorySSAUpdater Updater(&MSSA); StoreC->moveBefore(StoreB); - MSSA.spliceMemoryAccessAbove(cast(MSSA.getMemoryAccess(StoreB)), - MSSA.getMemoryAccess(StoreC)); + Updater.moveBefore(cast(MSSA.getMemoryAccess(StoreC)), + cast(MSSA.getMemoryAccess(StoreB))); MSSA.verifyMemorySSA(); @@ -533,4 +756,43 @@ EXPECT_TRUE(MSSA.locallyDominates(MSSA.getMemoryAccess(StoreA1), MSSA.getMemoryAccess(StoreA2))); } -#endif + +TEST_F(MemorySSATest, Irreducible) { + // Create the equivalent of + // x = something + // if (...) + // goto second_loop_entry + // while (...) { + // second_loop_entry: + // } + // use(x) + + SmallVector Inserted; + IRBuilder<> B(C); + F = Function::Create( + FunctionType::get(B.getVoidTy(), {B.getInt8PtrTy()}, false), + GlobalValue::ExternalLinkage, "F", &M); + + // Make blocks + BasicBlock *IfBB = BasicBlock::Create(C, "if", F); + BasicBlock *LoopStartBB = BasicBlock::Create(C, "loopstart", F); + BasicBlock *LoopMainBB = BasicBlock::Create(C, "loopmain", F); + BasicBlock *AfterLoopBB = BasicBlock::Create(C, "afterloop", F); + B.SetInsertPoint(IfBB); + B.CreateCondBr(B.getTrue(), LoopMainBB, LoopStartBB); + B.SetInsertPoint(LoopStartBB); + B.CreateBr(LoopMainBB); + B.SetInsertPoint(LoopMainBB); + B.CreateCondBr(B.getTrue(), LoopStartBB, AfterLoopBB); + B.SetInsertPoint(AfterLoopBB); + Argument *FirstArg = &*F->arg_begin(); + setupAnalyses(); + MemorySSA &MSSA = *Analyses->MSSA; + MemorySSAUpdater Updater(&MSSA); + // Create the load memory acccess + LoadInst *LoadInst = B.CreateLoad(FirstArg); + MemoryUse *LoadAccess = cast(MSSA.createMemoryAccessInBB( + LoadInst, nullptr, AfterLoopBB, MemorySSA::Beginning)); + Updater.insertUse(LoadAccess); + MSSA.verifyMemorySSA(); +}