[PATCH] D19821: [EarlyCSE] Optionally use MemorySSA. NFC.

Geoff Berry via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 23 10:17:31 PDT 2016


gberry added inline comments.

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:537
@@ +536,3 @@
+  MemoryAccess *LaterDef =
+      MSSA->getWalker()->getClobberingMemoryAccess(LaterInst);
+  return MSSA->dominates(LaterDef, MSSA->getMemoryAccess(EarlierInst));
----------------
dberlin wrote:
> gberry wrote:
> > dberlin wrote:
> > > 1. For loads, you don't have to ask for the clobbering access. It's already optimized such that getDefiningAccess == the clobbering access
> > > 
> > > 2. For stores, not sure if you realize this, but 
> > > 
> > > given
> > > store q  (lets's call this a)
> > > x = load p
> > > store q (let's call this b)
> > > 
> > > 
> > > if you call getClobberingMemoryAccess on b, it will return a.
> > > 
> > > 
> > For 1., I was not clear on whether this holds true after store removal.
> > 
> > For 2., yeah I get this, I'm not sure what you're getting at though.  The removal of this second store by EarlyCSE doesn't use MemorySSA to check for intervening loads in this change.  It uses the 'LastStore' tracking to know when a store made redundant by a second store can be removed.
> 1. Updates have to make it hold after store removal :)
> 
> The problem is that if we don't keep this invariant up to date, it means everyone uses getClobberingAccess, which does a bunch of work to discover the load already points to the same thing.
> 
> Everyone doing that is much higher than the cost of one person updating the dominating def.
> 
> (there is one case were getClobberingAccess will give you a better answer, and that is on cases where we gave up during use optimization. I only have one testcase this occurs on.  We only give up on optimizing a load if it's going to be super expensive, and you probably do *not* want to try to get better answers in that case).
> 
> As for updating when you remove stores, you should simply be able to replace any loads the store uses with getClobberingAccess(store) using RAUW.
> 
> Under the covers, removeMemoryAccess calls RAUW with the DefiningAccess.
> We could change it to use getClobberingMemoryAccess for loads, and DefiningAccess for stores.
> 
> 2. ah, okay.
> 
> 
Okay, I get why just checking the defining access for loads is better (we get to skip the AA check).
For stores, we may be able to do something faster than calling getClobberingAccess(store).  We could instead walk up the store defining access chain and stop if we get to a point that dominates the earlier load or a clobbering write.  I'll have to time this to see if it makes a difference.  I guess it will depend on what percentage of the time the clobber cache has been thrown away.

As for updating when removing stores: it seems like doing RAUW getClobberingAccess(store) is not optimal in some cases.  For example:

  store @G1 ; 1 = MD(entry)
  store @G2 ; 2 = MD(1)
  store %p ; 3 = MD(2)
  load @G1 ; MU(3)
  load @G2  ; MU(3)

If we remove 3 and RUAW getClobberingAccess(3) (=2) we get:

  store @G1 ; 1 = MD(entry)
  store @G2 ; 2 = MD(1)
  load @G1 ; MU(2)
  load @G2  ; MU(2)

but the load @G1 would be more precise if it was MU(1) (and the invariant that defining access == clobbering access would be broken).  Is this just a compile-time/precision trade-off?  Maybe for that reason it makes more sense to let the client decide if they want to do the simple RAUW getClobberingAccess(Store) or optimize each use separately?



https://reviews.llvm.org/D19821





More information about the llvm-commits mailing list