[llvm] r275940 - [MemorySSA] Update to the new shiny walker.
George Burgess IV via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 21 16:08:46 PDT 2016
Ahh, I see. Sounds good, then. :)
On Thu, Jul 21, 2016 at 4:07 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:
>
> On Jul 21, 2016, at 4:04 PM, George Burgess IV <
> george.burgess.iv at gmail.com> wrote:
>
> Thanks for bringing this up! And yeah, the first thing that `def_chain`
> should iterate over is `Target` itself. So, as long as `Target != nullptr`
> (which should always be true), we should be fine here.
>
> Is there something we could add to make coverity happy?
>
>
> I don’t know if we can add something, but I’m not even sure we “should”.
> It is reporting issue on new code every week or so, which is enough to
> review newly added code, and our existing codebase have thousands of such
> “errors” in Coverity.
>
> —
> Mehdi
>
>
>
> On Thu, Jul 21, 2016 at 3:50 PM, Mehdi Amini <mehdi.amini at apple.com>
> wrote:
>
>>
>> > On Jul 18, 2016, at 6:29 PM, George Burgess IV via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>> >
>> > Author: gbiv
>> > Date: Mon Jul 18 20:29:15 2016
>> > New Revision: 275940
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=275940&view=rev
>> > Log:
>> > [MemorySSA] Update to the new shiny walker.
>> >
>> > This patch updates MemorySSA's use-optimizing walker to be more
>> > accurate and, in some cases, faster.
>> >
>> > Essentially, this changed our core walking algorithm from a
>> > cache-as-you-go DFS to an iteratively expanded DFS, with all of the
>> > caching happening at the end. Said expansion happens when we hit a Phi,
>> > P; we'll try to do the smallest amount of work possible to see if
>> > optimizing above that Phi is legal in the first place. If so, we'll
>> > expand the search to see if we can optimize to the next phi, etc.
>> >
>> > An iteratively expanded DFS lets us potentially quit earlier (because we
>> > don't assume that we can optimize above all phis) than our old walker.
>> > Additionally, because we don't cache as we go, we can now optimize above
>> > loops.
>> >
>> > As an added bonus, this patch adds a ton of verification (if
>> > EXPENSIVE_CHECKS are enabled), so finding bugs is easier.
>> >
>> > Differential Revision: https://reviews.llvm.org/D21777
>> >
>> > Modified:
>> > llvm/trunk/include/llvm/Transforms/Utils/MemorySSA.h
>> > llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp
>> > llvm/trunk/test/Transforms/Util/MemorySSA/cyclicphi.ll
>> > llvm/trunk/test/Transforms/Util/MemorySSA/phi-translation.ll
>> >
>> > Modified: llvm/trunk/include/llvm/Transforms/Utils/MemorySSA.h
>> > URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Transforms/Utils/MemorySSA.h?rev=275940&r1=275939&r2=275940&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/include/llvm/Transforms/Utils/MemorySSA.h (original)
>> > +++ llvm/trunk/include/llvm/Transforms/Utils/MemorySSA.h Mon Jul 18
>> 20:29:15 2016
>> > @@ -580,6 +580,10 @@ public:
>> > /// whether MemoryAccess \p A dominates MemoryAccess \p B.
>> > bool locallyDominates(const MemoryAccess *A, const MemoryAccess *B)
>> const;
>> >
>> > + /// \brief Given two memory accesses in potentially different blocks,
>> > + /// determine whether MemoryAccess \p A dominates MemoryAccess \p B.
>> > + bool dominates(const MemoryAccess *A, const MemoryAccess *B) const;
>> > +
>> > /// \brief Verify that MemorySSA is self consistent (IE definitions
>> dominate
>> > /// all uses, uses appear in the right places). This is used by unit
>> tests.
>> > void verifyMemorySSA() const;
>> > @@ -594,6 +598,8 @@ protected:
>> >
>> > private:
>> > class CachingWalker;
>> > +
>> > + CachingWalker *getWalkerImpl();
>> > void buildMemorySSA();
>> > void verifyUseInDefs(MemoryAccess *, MemoryAccess *) const;
>> > using AccessMap = DenseMap<const BasicBlock *,
>> std::unique_ptr<AccessList>>;
>> >
>> > Modified: llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp
>> > URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp?rev=275940&r1=275939&r2=275940&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp (original)
>> > +++ llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp Mon Jul 18 20:29:15
>> 2016
>> > @@ -17,6 +17,7 @@
>> > #include "llvm/ADT/GraphTraits.h"
>> > #include "llvm/ADT/PostOrderIterator.h"
>> > #include "llvm/ADT/STLExtras.h"
>> > +#include "llvm/ADT/SmallBitVector.h"
>> > #include "llvm/ADT/SmallPtrSet.h"
>> > #include "llvm/ADT/SmallSet.h"
>> > #include "llvm/ADT/Statistic.h"
>> > @@ -86,7 +87,777 @@ public:
>> > OS << "; " << *MA << "\n";
>> > }
>> > };
>> > +}
>> > +
>> > +namespace {
>> > +struct UpwardsMemoryQuery {
>> > + // True if our original query started off as a call
>> > + bool IsCall;
>> > + // The pointer location we started the query with. This will be
>> empty if
>> > + // IsCall is true.
>> > + MemoryLocation StartingLoc;
>> > + // This is the instruction we were querying about.
>> > + const Instruction *Inst;
>> > + // The MemoryAccess we actually got called with, used to test local
>> domination
>> > + const MemoryAccess *OriginalAccess;
>> > +
>> > + UpwardsMemoryQuery()
>> > + : IsCall(false), Inst(nullptr), OriginalAccess(nullptr) {}
>> > +
>> > + UpwardsMemoryQuery(const Instruction *Inst, const MemoryAccess
>> *Access)
>> > + : IsCall(ImmutableCallSite(Inst)), Inst(Inst),
>> OriginalAccess(Access) {
>> > + if (!IsCall)
>> > + StartingLoc = MemoryLocation::get(Inst);
>> > + }
>> > +};
>> > +
>> > +static bool instructionClobbersQuery(MemoryDef *MD, const
>> MemoryLocation &Loc,
>> > + const UpwardsMemoryQuery &Query,
>> > + AliasAnalysis &AA) {
>> > + Instruction *DefMemoryInst = MD->getMemoryInst();
>> > + assert(DefMemoryInst && "Defining instruction not actually an
>> instruction");
>> > +
>> > + if (!Query.IsCall)
>> > + return AA.getModRefInfo(DefMemoryInst, Loc) & MRI_Mod;
>> > +
>> > + ModRefInfo I = AA.getModRefInfo(DefMemoryInst,
>> ImmutableCallSite(Query.Inst));
>> > + return I != MRI_NoModRef;
>> > +}
>> > +
>> > +/// Cache for our caching MemorySSA walker.
>> > +class WalkerCache {
>> > + DenseMap<ConstMemoryAccessPair, MemoryAccess *> Accesses;
>> > + DenseMap<const MemoryAccess *, MemoryAccess *> Calls;
>> > +
>> > +public:
>> > + MemoryAccess *lookup(const MemoryAccess *MA, const MemoryLocation
>> &Loc,
>> > + bool IsCall) const {
>> > + ++NumClobberCacheLookups;
>> > + MemoryAccess *R = IsCall ? Calls.lookup(MA) : Accesses.lookup({MA,
>> Loc});
>> > + if (R)
>> > + ++NumClobberCacheHits;
>> > + return R;
>> > + }
>> > +
>> > + bool insert(const MemoryAccess *MA, MemoryAccess *To,
>> > + const MemoryLocation &Loc, bool IsCall) {
>> > + // This is fine for Phis, since there are times where we can't
>> optimize
>> > + // them. Making a def its own clobber is never correct, though.
>> > + assert((MA != To || isa<MemoryPhi>(MA)) &&
>> > + "Something can't clobber itself!");
>> > +
>> > + ++NumClobberCacheInserts;
>> > + bool Inserted;
>> > + if (IsCall)
>> > + Inserted = Calls.insert({MA, To}).second;
>> > + else
>> > + Inserted = Accesses.insert({{MA, Loc}, To}).second;
>> > +
>> > + return Inserted;
>> > + }
>> > +
>> > + bool remove(const MemoryAccess *MA, const MemoryLocation &Loc, bool
>> IsCall) {
>> > + return IsCall ? Calls.erase(MA) : Accesses.erase({MA, Loc});
>> > + }
>> > +
>> > + void clear() {
>> > + Accesses.clear();
>> > + Calls.clear();
>> > + }
>> > +
>> > + bool contains(const MemoryAccess *MA) const {
>> > + for (auto &P : Accesses)
>> > + if (P.first.first == MA || P.second == MA)
>> > + return true;
>> > + for (auto &P : Calls)
>> > + if (P.first == MA || P.second == MA)
>> > + return true;
>> > + return false;
>> > + }
>> > +};
>> > +
>> > +/// Walks the defining uses of MemoryDefs. Stops after we hit
>> something that has
>> > +/// no defining use (e.g. a MemoryPhi or liveOnEntry). Note that, when
>> comparing
>> > +/// against a null def_chain_iterator, this will compare equal only
>> after
>> > +/// walking said Phi/liveOnEntry.
>> > +struct def_chain_iterator
>> > + : public iterator_facade_base<def_chain_iterator,
>> std::forward_iterator_tag,
>> > + MemoryAccess *> {
>> > + def_chain_iterator() : MA(nullptr) {}
>> > + def_chain_iterator(MemoryAccess *MA) : MA(MA) {}
>> > +
>> > + MemoryAccess *operator*() const { return MA; }
>> > +
>> > + def_chain_iterator &operator++() {
>> > + // N.B. liveOnEntry has a null defining access.
>> > + if (auto *MUD = dyn_cast<MemoryUseOrDef>(MA))
>> > + MA = MUD->getDefiningAccess();
>> > + else
>> > + MA = nullptr;
>> > + return *this;
>> > + }
>> > +
>> > + bool operator==(const def_chain_iterator &O) const { return MA ==
>> O.MA <http://o.ma/>; }
>> > +
>> > +private:
>> > + MemoryAccess *MA;
>> > +};
>> > +
>> > +static iterator_range<def_chain_iterator>
>> > +def_chain(MemoryAccess *MA, MemoryAccess *UpTo = nullptr) {
>> > +#ifdef EXPENSIVE_CHECKS
>> > + assert((!UpTo || find(def_chain(MA), UpTo) != def_chain_iterator())
>> &&
>> > + "UpTo isn't in the def chain!");
>> > +#endif
>> > + return make_range(def_chain_iterator(MA), def_chain_iterator(UpTo));
>> > +}
>> > +
>> > +/// Verifies that `Start` is clobbered by `ClobberAt`, and that nothing
>> > +/// inbetween `Start` and `ClobberAt` can clobbers `Start`.
>> > +///
>> > +/// This is meant to be as simple and self-contained as possible.
>> Because it
>> > +/// uses no cache, etc., it can be relatively expensive.
>> > +///
>> > +/// \param Start The MemoryAccess that we want to walk from.
>> > +/// \param ClobberAt A clobber for Start.
>> > +/// \param StartLoc The MemoryLocation for Start.
>> > +/// \param MSSA The MemorySSA isntance that Start and ClobberAt
>> belong to.
>> > +/// \param Query The UpwardsMemoryQuery we used for our search.
>> > +/// \param AA The AliasAnalysis we used for our search.
>> > +static void LLVM_ATTRIBUTE_UNUSED
>> > +checkClobberSanity(MemoryAccess *Start, MemoryAccess *ClobberAt,
>> > + const MemoryLocation &StartLoc, const MemorySSA
>> &MSSA,
>> > + const UpwardsMemoryQuery &Query, AliasAnalysis &AA)
>> {
>> > + assert(MSSA.dominates(ClobberAt, Start) && "Clobber doesn't dominate
>> start?");
>> > +
>> > + if (MSSA.isLiveOnEntryDef(Start)) {
>> > + assert(MSSA.isLiveOnEntryDef(ClobberAt) &&
>> > + "liveOnEntry must clobber itself");
>> > + return;
>> > + }
>> > +
>> > + assert((isa<MemoryPhi>(Start) || Start != ClobberAt) &&
>> > + "Start can't clobber itself!");
>> > +
>> > + bool FoundClobber = false;
>> > + DenseSet<MemoryAccessPair> VisitedPhis;
>> > + SmallVector<MemoryAccessPair, 8> Worklist;
>> > + Worklist.emplace_back(Start, StartLoc);
>> > + // Walk all paths from Start to ClobberAt, while looking for
>> clobbers. If one
>> > + // is found, complain.
>> > + while (!Worklist.empty()) {
>> > + MemoryAccessPair MAP = Worklist.pop_back_val();
>> > + // All we care about is that nothing from Start to ClobberAt
>> clobbers Start.
>> > + // We learn nothing from revisiting nodes.
>> > + if (!VisitedPhis.insert(MAP).second)
>> > + continue;
>> > +
>> > + for (MemoryAccess *MA : def_chain(MAP.first)) {
>> > + if (MA == ClobberAt) {
>> > + if (auto *MD = dyn_cast<MemoryDef>(MA)) {
>> > + // instructionClobbersQuery isn't essentially free, so don't
>> use `|=`,
>> > + // since it won't let us short-circuit.
>> > + //
>> > + // Also, note that this can't be hoisted out of the
>> `Worklist` loop,
>> > + // since MD may only act as a clobber for 1 of N
>> MemoryLocations.
>> > + FoundClobber = FoundClobber || MSSA.isLiveOnEntryDef(MD) ||
>> > + instructionClobbersQuery(MD, MAP.second,
>> Query, AA);
>> > + }
>> > + break;
>> > + }
>> > +
>> > + // We should never hit liveOnEntry, unless it's the clobber.
>> > + assert(!MSSA.isLiveOnEntryDef(MA) && "Hit liveOnEntry before
>> clobber?");
>> > +
>> > + if (auto *MD = dyn_cast<MemoryDef>(MA)) {
>> > + (void)MD;
>> > + assert(!instructionClobbersQuery(MD, MAP.second, Query, AA) &&
>> > + "Found clobber before reaching ClobberAt!");
>> > + continue;
>> > + }
>> > +
>> > + assert(isa<MemoryPhi>(MA));
>> > + Worklist.append(upward_defs_begin({MA, MAP.second}),
>> upward_defs_end());
>> > + }
>> > + }
>> > +
>> > + // If ClobberAt is a MemoryPhi, we can assume something above it
>> acted as a
>> > + // clobber. Otherwise, `ClobberAt` should've acted as a clobber at
>> some point.
>> > + assert((isa<MemoryPhi>(ClobberAt) || FoundClobber) &&
>> > + "ClobberAt never acted as a clobber");
>> > +}
>> > +
>> > +/// Our algorithm for walking (and trying to optimize) clobbers, all
>> wrapped up
>> > +/// in one class.
>> > +class ClobberWalker {
>> > + /// Save a few bytes by using unsigned instead of size_t.
>> > + using ListIndex = unsigned;
>> > +
>> > + /// Represents a span of contiguous MemoryDefs, potentially ending
>> in a
>> > + /// MemoryPhi.
>> > + struct DefPath {
>> > + MemoryLocation Loc;
>> > + // Note that, because we always walk in reverse, Last will always
>> dominate
>> > + // First. Also note that First and Last are inclusive.
>> > + MemoryAccess *First;
>> > + MemoryAccess *Last;
>> > + // N.B. Blocker is currently basically unused. The goal is to use
>> it to make
>> > + // cache invalidation better, but we're not there yet.
>> > + MemoryAccess *Blocker;
>> > + Optional<ListIndex> Previous;
>> > +
>> > + DefPath(const MemoryLocation &Loc, MemoryAccess *First,
>> MemoryAccess *Last,
>> > + Optional<ListIndex> Previous)
>> > + : Loc(Loc), First(First), Last(Last), Previous(Previous) {}
>> > +
>> > + DefPath(const MemoryLocation &Loc, MemoryAccess *Init,
>> > + Optional<ListIndex> Previous)
>> > + : DefPath(Loc, Init, Init, Previous) {}
>> > + };
>> > +
>> > + const MemorySSA &MSSA;
>> > + AliasAnalysis &AA;
>> > + DominatorTree &DT;
>> > + WalkerCache &WC;
>> > + UpwardsMemoryQuery *Query;
>> > + bool UseCache;
>> > +
>> > + // Phi optimization bookkeeping
>> > + SmallVector<DefPath, 32> Paths;
>> > + DenseSet<ConstMemoryAccessPair> VisitedPhis;
>> > + DenseMap<const BasicBlock *, MemoryAccess *> WalkTargetCache;
>> > +
>> > + void setUseCache(bool Use) { UseCache = Use; }
>> > + bool shouldIgnoreCache() const {
>> > + // UseCache will only be false when we're debugging, or when
>> expensive
>> > + // checks are enabled. In either case, we don't care deeply about
>> speed.
>> > + return LLVM_UNLIKELY(!UseCache);
>> > + }
>> > +
>> > + void addCacheEntry(const MemoryAccess *What, MemoryAccess *To,
>> > + const MemoryLocation &Loc) const {
>> > +// EXPENSIVE_CHECKS because most of these queries are redundant, and
>> if What
>> > +// and To are in the same BB, that gives us n^2 behavior.
>> > +#ifdef EXPENSIVE_CHECKS
>> > + assert(MSSA.dominates(To, What));
>> > +#endif
>> > + if (shouldIgnoreCache())
>> > + return;
>> > + WC.insert(What, To, Loc, Query->IsCall);
>> > + }
>> > +
>> > + MemoryAccess *lookupCache(const MemoryAccess *MA, const
>> MemoryLocation &Loc) {
>> > + return shouldIgnoreCache() ? nullptr : WC.lookup(MA, Loc,
>> Query->IsCall);
>> > + }
>> > +
>> > + void cacheDefPath(const DefPath &DN, MemoryAccess *Target) const {
>> > + if (shouldIgnoreCache())
>> > + return;
>> > +
>> > + for (MemoryAccess *MA : def_chain(DN.First, DN.Last))
>> > + addCacheEntry(MA, Target, DN.Loc);
>> > +
>> > + // DefPaths only express the path we walked. So, DN.Last could
>> either be a
>> > + // thing we want to cache, or not.
>> > + if (DN.Last != Target)
>> > + addCacheEntry(DN.Last, Target, DN.Loc);
>> > + }
>> > +
>> > + /// Find the nearest def or phi that `From` can legally be optimized
>> to.
>> > + ///
>> > + /// FIXME: Deduplicate this with MSSA::findDominatingDef. Ideally,
>> MSSA should
>> > + /// keep track of this information for us, and allow us O(1) lookups
>> of this
>> > + /// info.
>> > + MemoryAccess *getWalkTarget(const MemoryPhi *From) {
>> > + assert(!MSSA.isLiveOnEntryDef(From) && "liveOnEntry has no
>> target.");
>> > + assert(From->getNumOperands() && "Phi with no operands?");
>> > +
>> > + BasicBlock *BB = From->getBlock();
>> > + auto At = WalkTargetCache.find(BB);
>> > + if (At != WalkTargetCache.end())
>> > + return At->second;
>> > +
>> > + SmallVector<const BasicBlock *, 8> ToCache;
>> > + ToCache.push_back(BB);
>> > +
>> > + MemoryAccess *Result = MSSA.getLiveOnEntryDef();
>> > + DomTreeNode *Node = DT.getNode(BB);
>> > + while ((Node = Node->getIDom())) {
>> > + auto At = WalkTargetCache.find(BB);
>> > + if (At != WalkTargetCache.end()) {
>> > + Result = At->second;
>> > + break;
>> > + }
>> > +
>> > + auto *Accesses = MSSA.getBlockAccesses(Node->getBlock());
>> > + if (Accesses) {
>> > + auto Iter = find_if(reverse(*Accesses), [](const MemoryAccess
>> &MA) {
>> > + return !isa<MemoryUse>(MA);
>> > + });
>> > + if (Iter != Accesses->rend()) {
>> > + Result = const_cast<MemoryAccess *>(&*Iter);
>> > + break;
>> > + }
>> > + }
>> > +
>> > + ToCache.push_back(Node->getBlock());
>> > + }
>> > +
>> > + for (const BasicBlock *BB : ToCache)
>> > + WalkTargetCache.insert({BB, Result});
>> > + return Result;
>> > + }
>> > +
>> > + /// Result of calling walkToPhiOrClobber.
>> > + struct UpwardsWalkResult {
>> > + /// The "Result" of the walk. Either a clobber, the last thing we
>> walked, or
>> > + /// both.
>> > + MemoryAccess *Result;
>> > + bool IsKnownClobber;
>> > + bool FromCache;
>> > + };
>> > +
>> > + /// Walk to the next Phi or Clobber in the def chain starting at
>> Desc.Last.
>> > + /// This will update Desc.Last as it walks. It will (optionally)
>> also stop at
>> > + /// StopAt.
>> > + ///
>> > + /// This does not test for whether StopAt is a clobber
>> > + UpwardsWalkResult walkToPhiOrClobber(DefPath &Desc,
>> > + MemoryAccess *StopAt = nullptr)
>> {
>> > + assert(!isa<MemoryUse>(Desc.Last) && "Uses don't exist in my
>> world");
>> > +
>> > + for (MemoryAccess *Current : def_chain(Desc.Last)) {
>> > + Desc.Last = Current;
>> > + if (Current == StopAt)
>> > + return {Current, false, false};
>> > +
>> > + if (auto *MD = dyn_cast<MemoryDef>(Current))
>> > + if (MSSA.isLiveOnEntryDef(MD) ||
>> > + instructionClobbersQuery(MD, Desc.Loc, *Query, AA))
>> > + return {MD, true, false};
>> > +
>> > + // Cache checks must be done last, because if Current is a
>> clobber, the
>> > + // cache will contain the clobber for Current.
>> > + if (MemoryAccess *MA = lookupCache(Current, Desc.Loc))
>> > + return {MA, true, true};
>> > + }
>> > +
>> > + assert(isa<MemoryPhi>(Desc.Last) &&
>> > + "Ended at a non-clobber that's not a phi?");
>> > + return {Desc.Last, false, false};
>> > + }
>> > +
>> > + void addSearches(MemoryPhi *Phi, SmallVectorImpl<ListIndex>
>> &PausedSearches,
>> > + ListIndex PriorNode) {
>> > + auto UpwardDefs = make_range(upward_defs_begin({Phi,
>> Paths[PriorNode].Loc}),
>> > + upward_defs_end());
>> > + for (const MemoryAccessPair &P : UpwardDefs) {
>> > + PausedSearches.push_back(Paths.size());
>> > + Paths.emplace_back(P.second, P.first, PriorNode);
>> > + }
>> > + }
>> > +
>> > + /// Represents a search that terminated after finding a clobber.
>> This clobber
>> > + /// may or may not be present in the path of defs from
>> LastNode..SearchStart,
>> > + /// since it may have been retrieved from cache.
>> > + struct TerminatedPath {
>> > + MemoryAccess *Clobber;
>> > + ListIndex LastNode;
>> > + };
>> > +
>> > + /// Get an access that keeps us from optimizing to the given phi.
>> > + ///
>> > + /// PausedSearches is an array of indices into the Paths array. Its
>> incoming
>> > + /// value is the indices of searches that stopped at the last phi
>> optimization
>> > + /// target. It's left in an unspecified state.
>> > + ///
>> > + /// If this returns None, NewPaused is a vector of searches that
>> terminated
>> > + /// at StopWhere. Otherwise, NewPaused is left in an unspecified
>> state.
>> > + Optional<ListIndex>
>> > + getBlockingAccess(MemoryAccess *StopWhere,
>> > + SmallVectorImpl<ListIndex> &PausedSearches,
>> > + SmallVectorImpl<ListIndex> &NewPaused,
>> > + SmallVectorImpl<TerminatedPath> &Terminated) {
>> > + assert(!PausedSearches.empty() && "No searches to continue?");
>> > +
>> > + // BFS vs DFS really doesn't make a difference here, so just do a
>> DFS with
>> > + // PausedSearches as our stack.
>> > + while (!PausedSearches.empty()) {
>> > + ListIndex PathIndex = PausedSearches.pop_back_val();
>> > + DefPath &Node = Paths[PathIndex];
>> > +
>> > + // If we've already visited this path with this MemoryLocation,
>> we don't
>> > + // need to do so again.
>> > + //
>> > + // NOTE: That we just drop these paths on the ground makes
>> caching
>> > + // behavior sporadic. e.g. given a diamond:
>> > + // A
>> > + // B C
>> > + // D
>> > + //
>> > + // ...If we walk D, B, A, C, we'll only cache the result of phi
>> > + // optimization for A, B, and D; C will be skipped because it
>> dies here.
>> > + // This arguably isn't the worst thing ever, since:
>> > + // - We generally query things in a top-down order, so if we
>> got below D
>> > + // without needing cache entries for {C, MemLoc}, then
>> chances are
>> > + // that those cache entries would end up ultimately unused.
>> > + // - We still cache things for A, so C only needs to walk up a
>> bit.
>> > + // If this behavior becomes problematic, we can fix without a
>> ton of extra
>> > + // work.
>> > + if (!VisitedPhis.insert({Node.Last, Node.Loc}).second)
>> > + continue;
>> > +
>> > + UpwardsWalkResult Res = walkToPhiOrClobber(Node,
>> /*StopAt=*/StopWhere);
>> > + if (Res.IsKnownClobber) {
>> > + assert(Res.Result != StopWhere || Res.FromCache);
>> > + // If this wasn't a cache hit, we hit a clobber when walking.
>> That's a
>> > + // failure.
>> > + if (!Res.FromCache || !MSSA.dominates(Res.Result, StopWhere))
>> > + return PathIndex;
>> > +
>> > + // Otherwise, it's a valid thing to potentially optimize to.
>> > + Terminated.push_back({Res.Result, PathIndex});
>> > + continue;
>> > + }
>> > +
>> > + if (Res.Result == StopWhere) {
>> > + // We've hit our target. Save this path off for if we want to
>> continue
>> > + // walking.
>> > + NewPaused.push_back(PathIndex);
>> > + continue;
>> > + }
>> > +
>> > + assert(!MSSA.isLiveOnEntryDef(Res.Result) && "liveOnEntry is a
>> clobber");
>> > + addSearches(cast<MemoryPhi>(Res.Result), PausedSearches,
>> PathIndex);
>> > + }
>> > +
>> > + return None;
>> > + }
>> > +
>> > + template <typename T, typename Walker>
>> > + struct generic_def_path_iterator
>> > + : public iterator_facade_base<generic_def_path_iterator<T,
>> Walker>,
>> > + std::forward_iterator_tag, T *> {
>> > + generic_def_path_iterator() : W(nullptr), N(None) {}
>> > + generic_def_path_iterator(Walker *W, ListIndex N) : W(W), N(N) {}
>> > +
>> > + T &operator*() const { return curNode(); }
>> > +
>> > + generic_def_path_iterator &operator++() {
>> > + N = curNode().Previous;
>> > + return *this;
>> > + }
>> > +
>> > + bool operator==(const generic_def_path_iterator &O) const {
>> > + if (N.hasValue() != O.N.hasValue())
>> > + return false;
>> > + return !N.hasValue() || *N == *O.N;
>> > + }
>> > +
>> > + private:
>> > + T &curNode() const { return W->Paths[*N]; }
>> > +
>> > + Walker *W;
>> > + Optional<ListIndex> N;
>> > + };
>> > +
>> > + using def_path_iterator = generic_def_path_iterator<DefPath,
>> ClobberWalker>;
>> > + using const_def_path_iterator =
>> > + generic_def_path_iterator<const DefPath, const ClobberWalker>;
>> > +
>> > + iterator_range<def_path_iterator> def_path(ListIndex From) {
>> > + return make_range(def_path_iterator(this, From),
>> def_path_iterator());
>> > + }
>> > +
>> > + iterator_range<const_def_path_iterator> const_def_path(ListIndex
>> From) const {
>> > + return make_range(const_def_path_iterator(this, From),
>> > + const_def_path_iterator());
>> > + }
>> > +
>> > + struct OptznResult {
>> > + /// The path that contains our result.
>> > + TerminatedPath PrimaryClobber;
>> > + /// The paths that we can legally cache back from, but that aren't
>> > + /// necessarily the result of the Phi optimization.
>> > + SmallVector<TerminatedPath, 4> OtherClobbers;
>> > + };
>> > +
>> > + ListIndex defPathIndex(const DefPath &N) const {
>> > + // The assert looks nicer if we don't need to do &N
>> > + const DefPath *NP = &N;
>> > + assert(!Paths.empty() && NP >= &Paths.front() && NP <=
>> &Paths.back() &&
>> > + "Out of bounds DefPath!");
>> > + return NP - &Paths.front();
>> > + }
>> > +
>> > + /// Try to optimize a phi as best as we can. Returns a SmallVector
>> of Paths
>> > + /// that act as legal clobbers. Note that this won't return *all*
>> clobbers.
>> > + ///
>> > + /// Phi optimization algorithm tl;dr:
>> > + /// - Find the earliest def/phi, A, we can optimize to
>> > + /// - Find if all paths from the starting memory access ultimately
>> reach A
>> > + /// - If not, optimization isn't possible.
>> > + /// - Otherwise, walk from A to another clobber or phi, A'.
>> > + /// - If A' is a def, we're done.
>> > + /// - If A' is a phi, try to optimize it.
>> > + ///
>> > + /// A path is a series of {MemoryAccess, MemoryLocation} pairs. A
>> path
>> > + /// terminates when a MemoryAccess that clobbers said MemoryLocation
>> is found.
>> > + OptznResult tryOptimizePhi(MemoryPhi *Phi, MemoryAccess *Start,
>> > + const MemoryLocation &Loc) {
>> > + assert(Paths.empty() && VisitedPhis.empty() &&
>> > + "Reset the optimization state.");
>> > +
>> > + Paths.emplace_back(Loc, Start, Phi, None);
>> > + // Stores how many "valid" optimization nodes we had prior to
>> calling
>> > + // addSearches/getBlockingAccess. Necessary for caching if we had
>> a blocker.
>> > + auto PriorPathsSize = Paths.size();
>> > +
>> > + SmallVector<ListIndex, 16> PausedSearches;
>> > + SmallVector<ListIndex, 8> NewPaused;
>> > + SmallVector<TerminatedPath, 4> TerminatedPaths;
>> > +
>> > + addSearches(Phi, PausedSearches, 0);
>> > +
>> > + // Moves the TerminatedPath with the "most dominated" Clobber to
>> the end of
>> > + // Paths.
>> > + auto MoveDominatedPathToEnd = [&](SmallVectorImpl<TerminatedPath>
>> &Paths) {
>> > + assert(!Paths.empty() && "Need a path to move");
>> > + // FIXME: This is technically n^2 (n = distance(DefPath.First,
>> > + // DefPath.Last)) because of local dominance checks.
>> > + auto Dom = Paths.begin();
>> > + for (auto I = std::next(Dom), E = Paths.end(); I != E; ++I)
>> > + if (!MSSA.dominates(I->Clobber, Dom->Clobber))
>> > + Dom = I;
>> > + auto Last = Paths.end() - 1;
>> > + if (Last != Dom)
>> > + std::iter_swap(Last, Dom);
>> > + };
>> > +
>> > + MemoryPhi *Current = Phi;
>> > + while (1) {
>> > + assert(!MSSA.isLiveOnEntryDef(Current) &&
>> > + "liveOnEntry wasn't treated as a clobber?");
>> > +
>> > + MemoryAccess *Target = getWalkTarget(Current);
>> > + // If a TerminatedPath doesn't dominate Target, then it wasn't a
>> legal
>> > + // optimization for the prior phi.
>> > + assert(all_of(TerminatedPaths, [&](const TerminatedPath &P) {
>> > + return MSSA.dominates(P.Clobber, Target);
>> > + }));
>> > +
>> > + // FIXME: This is broken, because the Blocker may be reported to
>> be
>> > + // liveOnEntry, and we'll happily wait for that to disappear
>> (read: never)
>> > + // For the moment, this is fine, since we do basically nothing
>> with
>> > + // blocker info.
>> > + if (Optional<ListIndex> Blocker = getBlockingAccess(
>> > + Target, PausedSearches, NewPaused, TerminatedPaths)) {
>> > + MemoryAccess *BlockingAccess = Paths[*Blocker].Last;
>> > + // Cache our work on the blocking node, since we know that's
>> correct.
>> > + cacheDefPath(Paths[*Blocker], BlockingAccess);
>> > +
>> > + // Find the node we started at. We can't search based on
>> N->Last, since
>> > + // we may have gone around a loop with a different
>> MemoryLocation.
>> > + auto Iter = find_if(def_path(*Blocker), [&](const DefPath &N) {
>> > + return defPathIndex(N) < PriorPathsSize;
>> > + });
>> > + assert(Iter != def_path_iterator());
>> > +
>> > + DefPath &CurNode = *Iter;
>> > + assert(CurNode.Last == Current);
>> > + CurNode.Blocker = BlockingAccess;
>> > +
>> > + // Two things:
>> > + // A. We can't reliably cache all of NewPaused back. Consider
>> a case
>> > + // where we have two paths in NewPaused; one of which can't
>> optimize
>> > + // above this phi, whereas the other can. If we cache the
>> second path
>> > + // back, we'll end up with suboptimal cache entries. We can
>> handle
>> > + // cases like this a bit better when we either try to find
>> all
>> > + // clobbers that block phi optimization, or when our cache
>> starts
>> > + // supporting unfinished searches.
>> > + // B. We can't reliably cache TerminatedPaths back here
>> without doing
>> > + // extra checks; consider a case like:
>> > + // T
>> > + // / \
>> > + // D C
>> > + // \ /
>> > + // S
>> > + // Where T is our target, C is a node with a clobber on it,
>> D is a
>> > + // diamond (with a clobber *only* on the left or right
>> node, N), and
>> > + // S is our start. Say we walk to D, through the node
>> opposite N
>> > + // (read: ignoring the clobber), and see a cache entry in
>> the top
>> > + // node of D. That cache entry gets put into
>> TerminatedPaths. We then
>> > + // walk up to C (N is later in our worklist), find the
>> clobber, and
>> > + // quit. If we append TerminatedPaths to OtherClobbers,
>> we'll cache
>> > + // the bottom part of D to the cached clobber, ignoring the
>> clobber
>> > + // in N. Again, this problem goes away if we start tracking
>> all
>> > + // blockers for a given phi optimization.
>> > + TerminatedPath Result{CurNode.Last, defPathIndex(CurNode)};
>> > + return {Result, {}};
>> > + }
>> > +
>> > + // If there's nothing left to search, then all paths led to
>> valid clobbers
>> > + // that we got from our cache; pick the nearest to the start,
>> and allow
>> > + // the rest to be cached back.
>> > + if (NewPaused.empty()) {
>> > + MoveDominatedPathToEnd(TerminatedPaths);
>> > + TerminatedPath Result = TerminatedPaths.pop_back_val();
>> > + return {Result, std::move(TerminatedPaths)};
>> > + }
>> > +
>> > + MemoryAccess *DefChainEnd = nullptr;
>> > + SmallVector<TerminatedPath, 4> Clobbers;
>> > + for (ListIndex Paused : NewPaused) {
>> > + UpwardsWalkResult WR = walkToPhiOrClobber(Paths[Paused]);
>> > + if (WR.IsKnownClobber)
>> > + Clobbers.push_back({WR.Result, Paused});
>> > + else
>> > + // Micro-opt: If we hit the end of the chain, save it.
>> > + DefChainEnd = WR.Result;
>> > + }
>> > +
>> > + if (!TerminatedPaths.empty()) {
>> > + // If we couldn't find the dominating phi/liveOnEntry in the
>> above loop,
>> > + // do it now.
>> > + if (!DefChainEnd)
>> > + for (MemoryAccess *MA : def_chain(Target)))
>> > + DefChainEnd = MA;
>>
>> Is it guaranteed that `def_chain(Target)` isn’t empty?
>> Otherwise you have nullptr deref on the next line (Reported by coverity
>> who can’t know this invariant).
>>
>> —
>> Mehdi
>>
>> > +
>> > + // If any of the terminated paths don't dominate the phi we'll
>> try to
>> > + // optimize, we need to figure out what they are and quit.
>> > + const BasicBlock *ChainBB = DefChainEnd->getBlock();
>> > + for (const TerminatedPath &TP : TerminatedPaths) {
>> > + // Because we know that DefChainEnd is as "high" as we can
>> go, we
>> > + // don't need local dominance checks; BB dominance is
>> sufficient.
>> > + if (DT.dominates(ChainBB, TP.Clobber->getBlock()))
>> > + Clobbers.push_back(TP);
>> > + }
>> > + }
>> > +
>> > + // If we have clobbers in the def chain, find the one closest to
>> Current
>> > + // and quit.
>> > + if (!Clobbers.empty()) {
>> > + MoveDominatedPathToEnd(Clobbers);
>> > + TerminatedPath Result = Clobbers.pop_back_val();
>> > + return {Result, std::move(Clobbers)};
>> > + }
>> > +
>> > + assert(all_of(NewPaused,
>> > + [&](ListIndex I) { return Paths[I].Last ==
>> DefChainEnd; }));
>> > +
>> > + // Because liveOnEntry is a clobber, this must be a phi.
>> > + auto *DefChainPhi = cast<MemoryPhi>(DefChainEnd);
>> > +
>> > + PriorPathsSize = Paths.size();
>> > + PausedSearches.clear();
>> > + for (ListIndex I : NewPaused)
>> > + addSearches(DefChainPhi, PausedSearches, I);
>> > + NewPaused.clear();
>> > +
>> > + Current = DefChainPhi;
>> > + }
>> > + }
>> > +
>> > + /// Caches everything in an OptznResult.
>> > + void cacheOptResult(const OptznResult &R) {
>> > + if (R.OtherClobbers.empty()) {
>> > + // If we're not going to be caching OtherClobbers, don't bother
>> with
>> > + // marking visited/etc.
>> > + for (const DefPath &N :
>> const_def_path(R.PrimaryClobber.LastNode))
>> > + cacheDefPath(N, R.PrimaryClobber.Clobber);
>> > + return;
>> > + }
>> > +
>> > + // PrimaryClobber is our answer. If we can cache anything back, we
>> need to
>> > + // stop caching when we visit PrimaryClobber.
>> > + SmallBitVector Visited(Paths.size());
>> > + for (const DefPath &N : const_def_path(R.PrimaryClobber.LastNode))
>> {
>> > + Visited[defPathIndex(N)] = true;
>> > + cacheDefPath(N, R.PrimaryClobber.Clobber);
>> > + }
>> > +
>> > + for (const TerminatedPath &P : R.OtherClobbers) {
>> > + for (const DefPath &N : const_def_path(P.LastNode)) {
>> > + ListIndex NIndex = defPathIndex(N);
>> > + if (Visited[NIndex])
>> > + break;
>> > + Visited[NIndex] = true;
>> > + cacheDefPath(N, P.Clobber);
>> > + }
>> > + }
>> > + }
>> > +
>> > + void verifyOptResult(const OptznResult &R) const {
>> > + assert(all_of(R.OtherClobbers, [&](const TerminatedPath &P) {
>> > + return MSSA.dominates(P.Clobber, R.PrimaryClobber.Clobber);
>> > + }));
>> > + }
>> > +
>> > + void resetPhiOptznState() {
>> > + Paths.clear();
>> > + VisitedPhis.clear();
>> > + }
>> > +
>> > +public:
>> > + ClobberWalker(const MemorySSA &MSSA, AliasAnalysis &AA,
>> DominatorTree &DT,
>> > + WalkerCache &WC)
>> > + : MSSA(MSSA), AA(AA), DT(DT), WC(WC), UseCache(true) {}
>> > +
>> > + void reset() { WalkTargetCache.clear(); }
>> > +
>> > + /// Finds the nearest clobber for the given query, optimizing phis if
>> > + /// possible.
>> > + MemoryAccess *findClobber(MemoryAccess *Start, UpwardsMemoryQuery &Q,
>> > + bool UseWalkerCache = true) {
>> > + setUseCache(UseWalkerCache);
>> > + Query = &Q;
>> > +
>> > + MemoryAccess *Current = Start;
>> > + // This walker pretends uses don't exist. If we're handed one,
>> silently grab
>> > + // its def. (This has the nice side-effect of ensuring we never
>> cache uses)
>> > + if (auto *MU = dyn_cast<MemoryUse>(Start))
>> > + Current = MU->getDefiningAccess();
>> > +
>> > + DefPath FirstDesc(Q.StartingLoc, Current, Current, None);
>> > + // Fast path for the overly-common case (no crazy phi optimization
>> > + // necessary)
>> > + UpwardsWalkResult WalkResult = walkToPhiOrClobber(FirstDesc);
>> > + if (WalkResult.IsKnownClobber) {
>> > + cacheDefPath(FirstDesc, WalkResult.Result);
>> > + return WalkResult.Result;
>> > + }
>> > +
>> > + OptznResult OptRes =
>> > + tryOptimizePhi(cast<MemoryPhi>(FirstDesc.Last), Current,
>> Q.StartingLoc);
>> > + verifyOptResult(OptRes);
>> > + cacheOptResult(OptRes);
>> > + resetPhiOptznState();
>> > +
>> > +#ifdef EXPENSIVE_CHECKS
>> > + checkClobberSanity(Current, OptRes.PrimaryClobber.Clobber,
>> Q.StartingLoc,
>> > + MSSA, Q, AA);
>> > +#endif
>> > + return OptRes.PrimaryClobber.Clobber;
>> > + }
>> > +};
>> > +
>> > +struct RenamePassData {
>> > + DomTreeNode *DTN;
>> > + DomTreeNode::const_iterator ChildIt;
>> > + MemoryAccess *IncomingVal;
>> >
>> > + RenamePassData(DomTreeNode *D, DomTreeNode::const_iterator It,
>> > + MemoryAccess *M)
>> > + : DTN(D), ChildIt(It), IncomingVal(M) {}
>> > + void swap(RenamePassData &RHS) {
>> > + std::swap(DTN, RHS.DTN);
>> > + std::swap(ChildIt, RHS.ChildIt);
>> > + std::swap(IncomingVal, RHS.IncomingVal);
>> > + }
>> > +};
>> > +} // anonymous namespace
>> > +
>> > +namespace llvm {
>> > /// \brief A MemorySSAWalker that does AA walks and caching of lookups
>> to
>> > /// disambiguate accesses.
>> > ///
>> > @@ -121,6 +892,13 @@ public:
>> > /// ret i32 %r
>> > /// }
>> > class MemorySSA::CachingWalker final : public MemorySSAWalker {
>> > + WalkerCache Cache;
>> > + ClobberWalker Walker;
>> > + bool AutoResetWalker;
>> > +
>> > + MemoryAccess *getClobberingMemoryAccess(MemoryAccess *,
>> UpwardsMemoryQuery &);
>> > + void verifyRemoved(MemoryAccess *);
>> > +
>> > public:
>> > CachingWalker(MemorySSA *, AliasAnalysis *, DominatorTree *);
>> > ~CachingWalker() override;
>> > @@ -130,50 +908,17 @@ public:
>> > MemoryLocation &) override;
>> > void invalidateInfo(MemoryAccess *) override;
>> >
>> > -protected:
>> > - struct UpwardsMemoryQuery;
>> > - MemoryAccess *doCacheLookup(const MemoryAccess *, const
>> UpwardsMemoryQuery &,
>> > - const MemoryLocation &);
>> > -
>> > - void doCacheInsert(const MemoryAccess *, MemoryAccess *,
>> > - const UpwardsMemoryQuery &, const MemoryLocation
>> &);
>> > -
>> > - void doCacheRemove(const MemoryAccess *, const UpwardsMemoryQuery &,
>> > - const MemoryLocation &);
>> > -
>> > -private:
>> > - MemoryAccessPair UpwardsDFSWalk(MemoryAccess *, const MemoryLocation
>> &,
>> > - UpwardsMemoryQuery &, bool);
>> > - MemoryAccess *getClobberingMemoryAccess(MemoryAccess *,
>> UpwardsMemoryQuery &);
>> > - bool instructionClobbersQuery(const MemoryDef *, UpwardsMemoryQuery
>> &,
>> > - const MemoryLocation &Loc) const;
>> > - void verifyRemoved(MemoryAccess *);
>> > - SmallDenseMap<ConstMemoryAccessPair, MemoryAccess *>
>> > - CachedUpwardsClobberingAccess;
>> > - DenseMap<const MemoryAccess *, MemoryAccess *>
>> CachedUpwardsClobberingCall;
>> > - AliasAnalysis *AA;
>> > - DominatorTree *DT;
>> > + /// Whether we call resetClobberWalker() after each time we
>> *actually* walk to
>> > + /// answer a clobber query.
>> > + void setAutoResetWalker(bool AutoReset) { AutoResetWalker =
>> AutoReset; }
>> > +
>> > + /// Drop the walker's persistent data structures. At the moment,
>> this means
>> > + /// "drop the walker's cache of BasicBlocks ->
>> > + /// earliest-MemoryAccess-we-can-optimize-to". This is necessary if
>> we're
>> > + /// going to have DT updates, if we remove MemoryAccesses, etc.
>> > + void resetClobberWalker() { Walker.reset(); }
>> > };
>> > -}
>> >
>> > -namespace {
>> > -struct RenamePassData {
>> > - DomTreeNode *DTN;
>> > - DomTreeNode::const_iterator ChildIt;
>> > - MemoryAccess *IncomingVal;
>> > -
>> > - RenamePassData(DomTreeNode *D, DomTreeNode::const_iterator It,
>> > - MemoryAccess *M)
>> > - : DTN(D), ChildIt(It), IncomingVal(M) {}
>> > - void swap(RenamePassData &RHS) {
>> > - std::swap(DTN, RHS.DTN);
>> > - std::swap(ChildIt, RHS.ChildIt);
>> > - std::swap(IncomingVal, RHS.IncomingVal);
>> > - }
>> > -};
>> > -}
>> > -
>> > -namespace llvm {
>> > /// \brief Rename a single basic block into MemorySSA form.
>> > /// Uses the standard SSA renaming algorithm.
>> > /// \returns The new incoming value.
>> > @@ -417,7 +1162,10 @@ void MemorySSA::buildMemorySSA() {
>> > SmallPtrSet<BasicBlock *, 16> Visited;
>> > renamePass(DT->getRootNode(), LiveOnEntryDef.get(), Visited);
>> >
>> > - MemorySSAWalker *Walker = getWalker();
>> > + CachingWalker *Walker = getWalkerImpl();
>> > +
>> > + // We're doing a batch of updates; don't drop useful caches between
>> them.
>> > + Walker->setAutoResetWalker(false);
>> >
>> > // Now optimize the MemoryUse's defining access to point to the
>> nearest
>> > // dominating clobbering def.
>> > @@ -437,6 +1185,9 @@ void MemorySSA::buildMemorySSA() {
>> > }
>> > }
>> >
>> > + Walker->setAutoResetWalker(true);
>> > + Walker->resetClobberWalker();
>> > +
>> > // Mark the uses in unreachable blocks as live on entry, so that they
>> go
>> > // somewhere.
>> > for (auto &BB : F)
>> > @@ -444,7 +1195,9 @@ void MemorySSA::buildMemorySSA() {
>> > markUnreachableAsLiveOnEntry(&BB);
>> > }
>> >
>> > -MemorySSAWalker *MemorySSA::getWalker() {
>> > +MemorySSAWalker *MemorySSA::getWalker() { return getWalkerImpl(); }
>> > +
>> > +MemorySSA::CachingWalker *MemorySSA::getWalkerImpl() {
>> > if (Walker)
>> > return Walker.get();
>> >
>> > @@ -820,7 +1573,6 @@ MemoryPhi *MemorySSA::getMemoryAccess(co
>> > /// \returns True if \p Dominator dominates \p Dominatee.
>> > bool MemorySSA::locallyDominates(const MemoryAccess *Dominator,
>> > const MemoryAccess *Dominatee) const {
>> > -
>> > assert((Dominator->getBlock() == Dominatee->getBlock()) &&
>> > "Asking for local domination when accesses are in different
>> blocks!");
>> >
>> > @@ -848,6 +1600,19 @@ bool MemorySSA::locallyDominates(const M
>> > [&](const MemoryAccess &MA) { return &MA ==
>> Dominatee; });
>> > }
>> >
>> > +bool MemorySSA::dominates(const MemoryAccess *Dominator,
>> > + const MemoryAccess *Dominatee) const {
>> > + if (Dominator == Dominatee)
>> > + return true;
>> > +
>> > + if (isLiveOnEntryDef(Dominatee))
>> > + return false;
>> > +
>> > + if (Dominator->getBlock() != Dominatee->getBlock())
>> > + return DT->dominates(Dominator->getBlock(), Dominatee->getBlock());
>> > + return locallyDominates(Dominator, Dominatee);
>> > +}
>> > +
>> > const static char LiveOnEntryStr[] = "liveOnEntry";
>> >
>> > void MemoryDef::print(raw_ostream &OS) const {
>> > @@ -978,41 +1743,12 @@ MemorySSAWalker::MemorySSAWalker(MemoryS
>> >
>> > MemorySSA::CachingWalker::CachingWalker(MemorySSA *M, AliasAnalysis *A,
>> > DominatorTree *D)
>> > - : MemorySSAWalker(M), AA(A), DT(D) {}
>> > + : MemorySSAWalker(M), Walker(*M, *A, *D, Cache),
>> > + AutoResetWalker(true) {}
>> >
>> > MemorySSA::CachingWalker::~CachingWalker() {}
>> >
>> > -struct MemorySSA::CachingWalker::UpwardsMemoryQuery {
>> > - // True if we saw a phi whose predecessor was a backedge
>> > - bool SawBackedgePhi;
>> > - // True if our original query started off as a call
>> > - bool IsCall;
>> > - // The pointer location we started the query with. This will be
>> empty if
>> > - // IsCall is true.
>> > - MemoryLocation StartingLoc;
>> > - // This is the instruction we were querying about.
>> > - const Instruction *Inst;
>> > - // Set of visited Instructions for this query.
>> > - DenseSet<MemoryAccessPair> Visited;
>> > - // Vector of visited call accesses for this query. This is separated
>> out
>> > - // because you can always cache and lookup the result of call
>> queries (IE when
>> > - // IsCall == true) for every call in the chain. The calls have no AA
>> location
>> > - // associated with them with them, and thus, no context dependence.
>> > - SmallVector<const MemoryAccess *, 32> VisitedCalls;
>> > - // The MemoryAccess we actually got called with, used to test local
>> domination
>> > - const MemoryAccess *OriginalAccess;
>> > -
>> > - UpwardsMemoryQuery()
>> > - : SawBackedgePhi(false), IsCall(false), Inst(nullptr),
>> > - OriginalAccess(nullptr) {}
>> > -
>> > - UpwardsMemoryQuery(const Instruction *Inst, const MemoryAccess
>> *Access)
>> > - : SawBackedgePhi(false), IsCall(ImmutableCallSite(Inst)),
>> Inst(Inst),
>> > - OriginalAccess(Access) {}
>> > -};
>> > -
>> > void MemorySSA::CachingWalker::invalidateInfo(MemoryAccess *MA) {
>> > -
>> > // TODO: We can do much better cache invalidation with differently
>> stored
>> > // caches. For now, for MemoryUses, we simply remove them
>> > // from the cache, and kill the entire call/non-call cache for
>> everything
>> > @@ -1026,217 +1762,33 @@ void MemorySSA::CachingWalker::invalidat
>> > // itself.
>> >
>> > if (MemoryUse *MU = dyn_cast<MemoryUse>(MA)) {
>> > - UpwardsMemoryQuery Q;
>> > - Instruction *I = MU->getMemoryInst();
>> > - Q.IsCall = bool(ImmutableCallSite(I));
>> > - Q.Inst = I;
>> > - if (!Q.IsCall)
>> > - Q.StartingLoc = MemoryLocation::get(I);
>> > - doCacheRemove(MA, Q, Q.StartingLoc);
>> > + UpwardsMemoryQuery Q(MU->getMemoryInst(), MU);
>> > + Cache.remove(MU, Q.StartingLoc, Q.IsCall);
>> > } else {
>> > // If it is not a use, the best we can do right now is destroy the
>> cache.
>> > - CachedUpwardsClobberingCall.clear();
>> > - CachedUpwardsClobberingAccess.clear();
>> > + Cache.clear();
>> > }
>> >
>> > #ifdef EXPENSIVE_CHECKS
>> > - // Run this only when expensive checks are enabled.
>> > verifyRemoved(MA);
>> > #endif
>> > }
>> >
>> > -void MemorySSA::CachingWalker::doCacheRemove(const MemoryAccess *M,
>> > - const UpwardsMemoryQuery
>> &Q,
>> > - const MemoryLocation
>> &Loc) {
>> > - if (Q.IsCall)
>> > - CachedUpwardsClobberingCall.erase(M);
>> > - else
>> > - CachedUpwardsClobberingAccess.erase({M, Loc});
>> > -}
>> > -
>> > -void MemorySSA::CachingWalker::doCacheInsert(const MemoryAccess *M,
>> > - MemoryAccess *Result,
>> > - const UpwardsMemoryQuery
>> &Q,
>> > - const MemoryLocation
>> &Loc) {
>> > - // This is fine for Phis, since there are times where we can't
>> optimize them.
>> > - // Making a def its own clobber is never correct, though.
>> > - assert((Result != M || isa<MemoryPhi>(M)) &&
>> > - "Something can't clobber itself!");
>> > - ++NumClobberCacheInserts;
>> > - if (Q.IsCall)
>> > - CachedUpwardsClobberingCall[M] = Result;
>> > - else
>> > - CachedUpwardsClobberingAccess[{M, Loc}] = Result;
>> > -}
>> > -
>> > -MemoryAccess *
>> > -MemorySSA::CachingWalker::doCacheLookup(const MemoryAccess *M,
>> > - const UpwardsMemoryQuery &Q,
>> > - const MemoryLocation &Loc) {
>> > - ++NumClobberCacheLookups;
>> > - MemoryAccess *Result;
>> > -
>> > - if (Q.IsCall)
>> > - Result = CachedUpwardsClobberingCall.lookup(M);
>> > - else
>> > - Result = CachedUpwardsClobberingAccess.lookup({M, Loc});
>> > -
>> > - if (Result)
>> > - ++NumClobberCacheHits;
>> > - return Result;
>> > -}
>> > -
>> > -bool MemorySSA::CachingWalker::instructionClobbersQuery(
>> > - const MemoryDef *MD, UpwardsMemoryQuery &Q,
>> > - const MemoryLocation &Loc) const {
>> > - Instruction *DefMemoryInst = MD->getMemoryInst();
>> > - assert(DefMemoryInst && "Defining instruction not actually an
>> instruction");
>> > -
>> > - if (!Q.IsCall)
>> > - return AA->getModRefInfo(DefMemoryInst, Loc) & MRI_Mod;
>> > -
>> > - // If this is a call, mark it for caching
>> > - if (ImmutableCallSite(DefMemoryInst))
>> > - Q.VisitedCalls.push_back(MD);
>> > - ModRefInfo I = AA->getModRefInfo(DefMemoryInst,
>> ImmutableCallSite(Q.Inst));
>> > - return I != MRI_NoModRef;
>> > -}
>> > -
>> > -MemoryAccessPair MemorySSA::CachingWalker::UpwardsDFSWalk(
>> > - MemoryAccess *StartingAccess, const MemoryLocation &Loc,
>> > - UpwardsMemoryQuery &Q, bool FollowingBackedge) {
>> > - MemoryAccess *ModifyingAccess = nullptr;
>> > -
>> > - auto DFI = df_begin(StartingAccess);
>> > - for (auto DFE = df_end(StartingAccess); DFI != DFE;) {
>> > - MemoryAccess *CurrAccess = *DFI;
>> > - if (MSSA->isLiveOnEntryDef(CurrAccess))
>> > - return {CurrAccess, Loc};
>> > - // If this is a MemoryDef, check whether it clobbers our current
>> query. This
>> > - // needs to be done before consulting the cache, because the cache
>> reports
>> > - // the clobber for CurrAccess. If CurrAccess is a clobber for this
>> query,
>> > - // and we ask the cache for information first, then we might skip
>> this
>> > - // clobber, which is bad.
>> > - if (auto *MD = dyn_cast<MemoryDef>(CurrAccess)) {
>> > - // If we hit the top, stop following this path.
>> > - // While we can do lookups, we can't sanely do inserts here
>> unless we were
>> > - // to track everything we saw along the way, since we don't know
>> where we
>> > - // will stop.
>> > - if (instructionClobbersQuery(MD, Q, Loc)) {
>> > - ModifyingAccess = CurrAccess;
>> > - break;
>> > - }
>> > - }
>> > - if (auto CacheResult = doCacheLookup(CurrAccess, Q, Loc))
>> > - return {CacheResult, Loc};
>> > -
>> > - // We need to know whether it is a phi so we can track backedges.
>> > - // Otherwise, walk all upward defs.
>> > - if (!isa<MemoryPhi>(CurrAccess)) {
>> > - ++DFI;
>> > - continue;
>> > - }
>> > -
>> > -#ifndef NDEBUG
>> > - // The loop below visits the phi's children for us. Because phis
>> are the
>> > - // only things with multiple edges, skipping the children should
>> always lead
>> > - // us to the end of the loop.
>> > - //
>> > - // Use a copy of DFI because skipChildren would kill our search
>> stack, which
>> > - // would make caching anything on the way back impossible.
>> > - auto DFICopy = DFI;
>> > - assert(DFICopy.skipChildren() == DFE &&
>> > - "Skipping phi's children doesn't end the DFS?");
>> > -#endif
>> > -
>> > - const MemoryAccessPair PHIPair(CurrAccess, Loc);
>> > -
>> > - // Don't try to optimize this phi again if we've already tried to
>> do so.
>> > - if (!Q.Visited.insert(PHIPair).second) {
>> > - ModifyingAccess = CurrAccess;
>> > - break;
>> > - }
>> > -
>> > - std::size_t InitialVisitedCallSize = Q.VisitedCalls.size();
>> > -
>> > - // Recurse on PHI nodes, since we need to change locations.
>> > - // TODO: Allow graphtraits on pairs, which would turn this whole
>> function
>> > - // into a normal single depth first walk.
>> > - MemoryAccess *FirstDef = nullptr;
>> > - for (auto MPI = upward_defs_begin(PHIPair), MPE =
>> upward_defs_end();
>> > - MPI != MPE; ++MPI) {
>> > - bool Backedge =
>> > - !FollowingBackedge &&
>> > - DT->dominates(CurrAccess->getBlock(), MPI.getPhiArgBlock());
>> > -
>> > - MemoryAccessPair CurrentPair =
>> > - UpwardsDFSWalk(MPI->first, MPI->second, Q, Backedge);
>> > - // All the phi arguments should reach the same point if we can
>> bypass
>> > - // this phi. The alternative is that they hit this phi node,
>> which
>> > - // means we can skip this argument.
>> > - if (FirstDef && CurrentPair.first != PHIPair.first &&
>> > - CurrentPair.first != FirstDef) {
>> > - ModifyingAccess = CurrAccess;
>> > - break;
>> > - }
>> > -
>> > - if (!FirstDef)
>> > - FirstDef = CurrentPair.first;
>> > - }
>> > -
>> > - // If we exited the loop early, go with the result it gave us.
>> > - if (!ModifyingAccess) {
>> > - assert(FirstDef && "Found a Phi with no upward defs?");
>> > - ModifyingAccess = FirstDef;
>> > - } else {
>> > - // If we can't optimize this Phi, then we can't safely cache any
>> of the
>> > - // calls we visited when trying to optimize it. Wipe them out
>> now.
>> > - Q.VisitedCalls.resize(InitialVisitedCallSize);
>> > - }
>> > - break;
>> > - }
>> > -
>> > - if (!ModifyingAccess)
>> > - return {MSSA->getLiveOnEntryDef(), Q.StartingLoc};
>> > -
>> > - const BasicBlock *OriginalBlock = StartingAccess->getBlock();
>> > - assert(DFI.getPathLength() > 0 && "We dropped our path?");
>> > - unsigned N = DFI.getPathLength();
>> > - // If we found a clobbering def, the last element in the path will
>> be our
>> > - // clobber, so we don't want to cache that to itself. OTOH, if we
>> optimized a
>> > - // phi, we can add the last thing in the path to the cache, since
>> that won't
>> > - // be the result.
>> > - if (DFI.getPath(N - 1) == ModifyingAccess)
>> > - --N;
>> > - for (; N > 1; --N) {
>> > - MemoryAccess *CacheAccess = DFI.getPath(N - 1);
>> > - BasicBlock *CurrBlock = CacheAccess->getBlock();
>> > - if (!FollowingBackedge)
>> > - doCacheInsert(CacheAccess, ModifyingAccess, Q, Loc);
>> > - if (DT->dominates(CurrBlock, OriginalBlock) &&
>> > - (CurrBlock != OriginalBlock || !FollowingBackedge ||
>> > - MSSA->locallyDominates(CacheAccess, StartingAccess)))
>> > - break;
>> > - }
>> > -
>> > - // Cache everything else on the way back. The caller should cache
>> > - // StartingAccess for us.
>> > - for (; N > 1; --N) {
>> > - MemoryAccess *CacheAccess = DFI.getPath(N - 1);
>> > - doCacheInsert(CacheAccess, ModifyingAccess, Q, Loc);
>> > - }
>> > - assert(Q.Visited.size() < 1000 && "Visited too much");
>> > -
>> > - return {ModifyingAccess, Loc};
>> > -}
>> > -
>> > /// \brief Walk the use-def chains starting at \p MA and find
>> > /// the MemoryAccess that actually clobbers Loc.
>> > ///
>> > /// \returns our clobbering memory access
>> > MemoryAccess *MemorySSA::CachingWalker::getClobberingMemoryAccess(
>> > MemoryAccess *StartingAccess, UpwardsMemoryQuery &Q) {
>> > - return UpwardsDFSWalk(StartingAccess, Q.StartingLoc, Q, false).first;
>> > + MemoryAccess *New = Walker.findClobber(StartingAccess, Q);
>> > +#ifdef EXPENSIVE_CHECKS
>> > + MemoryAccess *NewNoCache =
>> > + Walker.findClobber(StartingAccess, Q, /*UseWalkerCache=*/false);
>> > + assert(NewNoCache == New && "Cache made us hand back a different
>> result?");
>> > +#endif
>> > + if (AutoResetWalker)
>> > + resetClobberWalker();
>> > + return New;
>> > }
>> >
>> > MemoryAccess *MemorySSA::CachingWalker::getClobberingMemoryAccess(
>> > @@ -1258,10 +1810,10 @@ MemoryAccess *MemorySSA::CachingWalker::
>> > UpwardsMemoryQuery Q;
>> > Q.OriginalAccess = StartingUseOrDef;
>> > Q.StartingLoc = Loc;
>> > - Q.Inst = StartingUseOrDef->getMemoryInst();
>> > + Q.Inst = I;
>> > Q.IsCall = false;
>> >
>> > - if (auto CacheResult = doCacheLookup(StartingUseOrDef, Q,
>> Q.StartingLoc))
>> > + if (auto *CacheResult = Cache.lookup(StartingUseOrDef, Loc,
>> Q.IsCall))
>> > return CacheResult;
>> >
>> > // Unlike the other function, do not walk to the def of a def,
>> because we are
>> > @@ -1271,9 +1823,6 @@ MemoryAccess *MemorySSA::CachingWalker::
>> > : StartingUseOrDef;
>> >
>> > MemoryAccess *Clobber = getClobberingMemoryAccess(DefiningAccess, Q);
>> > - // Only cache this if it wouldn't make Clobber point to itself.
>> > - if (Clobber != StartingAccess)
>> > - doCacheInsert(Q.OriginalAccess, Clobber, Q, Q.StartingLoc);
>> > DEBUG(dbgs() << "Starting Memory SSA clobber for " << *I << " is ");
>> > DEBUG(dbgs() << *StartingUseOrDef << "\n");
>> > DEBUG(dbgs() << "Final Memory SSA clobber for " << *I << " is ");
>> > @@ -1287,21 +1836,14 @@ MemorySSA::CachingWalker::getClobberingM
>> > // access, since we only map BB's to PHI's. So, this must be a use or
>> def.
>> > auto *StartingAccess = cast<MemoryUseOrDef>(MSSA->getMemoryAccess(I));
>> >
>> > - bool IsCall = bool(ImmutableCallSite(I));
>> > -
>> > + UpwardsMemoryQuery Q(I, StartingAccess);
>> > // We can't sanely do anything with a fences, they conservatively
>> > // clobber all memory, and have no locations to get pointers from to
>> > // try to disambiguate.
>> > - if (!IsCall && I->isFenceLike())
>> > + if (!Q.IsCall && I->isFenceLike())
>> > return StartingAccess;
>> >
>> > - UpwardsMemoryQuery Q;
>> > - Q.OriginalAccess = StartingAccess;
>> > - Q.IsCall = IsCall;
>> > - if (!Q.IsCall)
>> > - Q.StartingLoc = MemoryLocation::get(I);
>> > - Q.Inst = I;
>> > - if (auto CacheResult = doCacheLookup(StartingAccess, Q,
>> Q.StartingLoc))
>> > + if (auto *CacheResult = Cache.lookup(StartingAccess, Q.StartingLoc,
>> Q.IsCall))
>> > return CacheResult;
>> >
>> > // Start with the thing we already think clobbers this location
>> > @@ -1313,18 +1855,6 @@ MemorySSA::CachingWalker::getClobberingM
>> > return DefiningAccess;
>> >
>> > MemoryAccess *Result = getClobberingMemoryAccess(DefiningAccess, Q);
>> > - // DFS won't cache a result for DefiningAccess. So, if
>> DefiningAccess isn't
>> > - // our clobber, be sure that it gets a cache entry, too.
>> > - if (Result != DefiningAccess)
>> > - doCacheInsert(DefiningAccess, Result, Q, Q.StartingLoc);
>> > - doCacheInsert(Q.OriginalAccess, Result, Q, Q.StartingLoc);
>> > - // TODO: When this implementation is more mature, we may want to
>> figure out
>> > - // what this additional caching buys us. It's most likely A Good
>> Thing.
>> > - if (Q.IsCall)
>> > - for (const MemoryAccess *MA : Q.VisitedCalls)
>> > - if (MA != Result)
>> > - doCacheInsert(MA, Result, Q, Q.StartingLoc);
>> > -
>> > DEBUG(dbgs() << "Starting Memory SSA clobber for " << *I << " is ");
>> > DEBUG(dbgs() << *DefiningAccess << "\n");
>> > DEBUG(dbgs() << "Final Memory SSA clobber for " << *I << " is ");
>> > @@ -1335,14 +1865,7 @@ MemorySSA::CachingWalker::getClobberingM
>> >
>> > // Verify that MA doesn't exist in any of the caches.
>> > void MemorySSA::CachingWalker::verifyRemoved(MemoryAccess *MA) {
>> > -#ifndef NDEBUG
>> > - for (auto &P : CachedUpwardsClobberingAccess)
>> > - assert(P.first.first != MA && P.second != MA &&
>> > - "Found removed MemoryAccess in cache.");
>> > - for (auto &P : CachedUpwardsClobberingCall)
>> > - assert(P.first != MA && P.second != MA &&
>> > - "Found removed MemoryAccess in cache.");
>> > -#endif // !NDEBUG
>> > + assert(!Cache.contains(MA) && "Found removed MemoryAccess in
>> cache.");
>> > }
>> >
>> > MemoryAccess *
>> > @@ -1359,4 +1882,4 @@ MemoryAccess *DoNothingMemorySSAWalker::
>> > return Use->getDefiningAccess();
>> > return StartingAccess;
>> > }
>> > -}
>> > +} // namespace llvm
>> >
>> > Modified: llvm/trunk/test/Transforms/Util/MemorySSA/cyclicphi.ll
>> > URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Util/MemorySSA/cyclicphi.ll?rev=275940&r1=275939&r2=275940&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/test/Transforms/Util/MemorySSA/cyclicphi.ll (original)
>> > +++ llvm/trunk/test/Transforms/Util/MemorySSA/cyclicphi.ll Mon Jul 18
>> 20:29:15 2016
>> > @@ -56,8 +56,7 @@ bb68:
>> >
>> > bb77: ; preds = %bb68, %bb26
>> > ; CHECK: 2 = MemoryPhi({bb26,3},{bb68,1})
>> > -; FIXME: This should be MemoryUse(liveOnEntry)
>> > -; CHECK: MemoryUse(3)
>> > +; CHECK: MemoryUse(liveOnEntry)
>> > ; CHECK-NEXT: %tmp78 = load i64*, i64** %tmp25, align 8
>> > %tmp78 = load i64*, i64** %tmp25, align 8
>> > br label %bb26
>> >
>> > Modified: llvm/trunk/test/Transforms/Util/MemorySSA/phi-translation.ll
>> > URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Util/MemorySSA/phi-translation.ll?rev=275940&r1=275939&r2=275940&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/test/Transforms/Util/MemorySSA/phi-translation.ll
>> (original)
>> > +++ llvm/trunk/test/Transforms/Util/MemorySSA/phi-translation.ll Mon
>> Jul 18 20:29:15 2016
>> > @@ -138,8 +138,7 @@ loop.3:
>> > ; CHECK: 4 = MemoryDef(5)
>> > ; CHECK-NEXT: store i8 2, i8* %p2
>> > store i8 2, i8* %p2
>> > -; FIXME: This should be MemoryUse(1)
>> > -; CHECK: MemoryUse(5)
>> > +; CHECK: MemoryUse(1)
>> > ; CHECK-NEXT: load i8, i8* %p1
>> > load i8, i8* %p1
>> > br i1 undef, label %loop.2, label %loop.1
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160721/cc4301cc/attachment.html>
More information about the llvm-commits
mailing list