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

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Only looked at the bit I was mentioned in.



================
Comment at: lib/Transforms/Scalar/LICM.cpp:846
+        // This is only called to properly update the defining access
+        MSSA->getWalker()->getClobberingMemoryAccess(NewMemUse);
+      }
----------------
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)


https://reviews.llvm.org/D40375