[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:1159
+      if (EnableLicmCap)
+        MSSA->getWalker()->getClobberingMemoryAccess(OldMemAcc);
+    }
----------------
asbirlea wrote:
> sanjoy wrote:
> > sanjoy wrote:
> > > I not convinced this is a good idea -- it looks like we're coding to the implementation of `getClobberingMemoryAccess` here instead of coding to its interface.  But if @george.burgess.iv is fine with it, I am too.
> > It looks like `MemorySSA::CachingWalker::getClobberingMemoryAccess` has an early exit if `OldMemAcc` was optimized.  Are we okay not updating the defining access in that case?
> If it's been optimized, then the defining access should have been updated, and viceversa.
> @george.burgess.iv: Could you clarify this please? I don't see resetOptimized being called on any path for the call chain starting with moreToPlace->insertDef, though it would make sense for this to happen. Am I missing something here?
> I not convinced this is a good idea -- it looks like we're coding to the implementation of getClobberingMemoryAccess here instead of coding to its interface. But if @george.burgess.iv is fine with it, I am too

In general, I agree, but Walkers are a sticky concept. The model we've sold people is "always call `getWalker()->getClobberingMemoryAddress(MA)` if you need the optimized access; the walker will handle making repeated calls fast," and we're specifically calling this on `MSSA->getWalker()`.

I'll try to find a way to structure MSSA so that this guarantee is more obvious.

> Am I missing something here?

Our theoretical `isOptimized` bit depends on two things: being flipped by a call to `setOptimized`, and either our defining access or optimized access having the same ID as it did when `setOptimized` was called. (...It also depends on stores not appearing out of thin air ;) )

So, when you move a Def, all Uses are implicitly invalided by the `replaceAllUsesWith` (since their defining access no longer has the same ID as before), and the Def's cached clobber is invalidated when you call `setDefiningAccess` on it.

Similarly, when you move a Use, we just set its defining access to whatever's closest. So, its cache will be invalidated (or not, if you move it right under its old clobber. But then the optimized use is still correct).

What I'm unsure about is what happens what happens to `Def`s optimized to a `Def` when said optimized-to `Def` is moved. I feel like I asked this during the original reviews for the caching/updater stuff, but I don't remember the answer. I'll do archaeology and get back to you. :)

(If the Def/Def case is uninteresting to this pass, feel free to not block this patch on my search)


Repository:
  rL LLVM

https://reviews.llvm.org/D40375