[PATCH] D40375: Use MemorySSA in LICM to do sinking and hoisting.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added inline comments.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:846
+        // This is only called to properly update the defining access
+        MSSA->getWalker()->getClobberingMemoryAccess(NewMemUse);
+      }
----------------
asbirlea wrote:
> george.burgess.iv wrote:
> > asbirlea wrote:
> > > sanjoy wrote:
> > > > I'm not clear on how this works -- IIUC you're calling into `getClobberingMemoryAccess` with `NewMemUse`'s `DefiningAccess` set to `nullptr`, but:
> > > > 
> > > >  - Why don't we have to do this for `MemoryDef`s?
> > > >  - What if `MSSA->getWalker()` is a `DoNothingMemorySSAWalker`?
> > > >  - There are places where we should be crashing, like: `CachingWalker::getClobberingMemoryAccess` line 2078 that calls `getClobberingMemoryAccess(DefiningAccess, Q)` that calls `ClobberWalker::findWalker` which has `if (auto *MU = dyn_cast<MemoryUse>(Start))`.
> > > AFAIU:
> > > For Defs the DefiningAccess is properly updated.
> > > For Uses, there is a DefiningAccess set, but that one is not optimized. When adding an access like in this case, calling getClobberingMemoryAccess will optimize it.
> > > If the walker is DoNothingMemorySSAWalker, it won't optimize?
> > > 
> > > Pulling in george.burgess.iv to clarify, it's possible I'm missing something here.
> > > 
> > > IIUC you're calling into getClobberingMemoryAccess with NewMemUse's DefiningAccess set to nullptr
> > 
> > I'm assuming you meant `MemUse`, in which case `insertUse` sets up the defining access to something non-null.
> > 
> > It's sort of sketchy that we're trying to eagerly update the defining access using the walker, though. Is there a reason that we can't call `MSSA->getWalker()->getClobberingMemoryAccess(MemUse);` when we need optimized clobbers? Doing so should be incredibly cheap when the use has already been optimized, and it lets us be lazy about the work we do (if we don't need an accurate clobber for `MemUse`, we'll just never compute it)
> > I'm assuming you meant MemUse, in which case insertUse sets up the defining access to something non-null.
> 
> Thanks for clarifying this.
> 
> > It's sort of sketchy that we're trying to eagerly update the defining access using the walker, though. Is there a reason that we can't call MSSA->getWalker()->getClobberingMemoryAccess(MemUse); when we need optimized clobbers? Doing so should be incredibly cheap when the use has already been optimized, and it lets us be lazy about the work we do (if we don't need an accurate clobber for MemUse, we'll just never compute it)
> 
> The reasoning here is that when making code changes such as hoist/sink, I need accesses to be (reasonably) optimized.  I'm not necessarily to looking to get full precision here, I'd be perfectly happy with an API to optimize up to the MemorySSA optimize-uses cap.
> In LICM.cpp:1652 (and in the future for promotion) I'm relying on getting the defining access, not the walker->clobbering access, allowing for some imprecision in exchange for not paying the lookup time for blocks with large load/store count. 
> I had come across a case where after a hoist or sink, this approach did not work for a very small testcase without using the walker to get clobbering.
> So the choice I made was to use the walker ahead of time to update accesses I insert when cloning, allowing me to use getDefining afterwards.
> 
> Hopefully I clarified the intention here :). Do you think there's a better way to address this problem? I'm open to suggestions.
> 
> @dberlin
> > So, this use renaming is expensive, and i'd like to understand the goal (It requires walking all stores and loads in the dominator subtree rooted at your memory access) It's really meant for when you've inserted defs that may alias existing uses.
> > If your goal is to replace all uses of  an old memory access with some new memoryaccess, you want to use the replace API :)
> The code here is creating a new memory access to handle cloning an access in all exit blocks, where that access may alias existing uses, so I think using the replace API isn't enough.
> Please let me know if there is a way to handle this differently.
(To be clear: the addition of the flag resolved my MemorySSA comments. While it's sort of hacky that we need to have it in the first place, the best place to handle "this {block,function} is pathologically big; let's give up instead of taking forever to walk it, and rewalk it, and rewalk it, and ..." is in MSSA, IMO. Until we make MSSA smart enough to actually do that, though, a targeted escape hatch seems like the best way forward to me.)


Repository:
  rL LLVM

https://reviews.llvm.org/D40375