[PATCH] D86486: [DSE,MemorySSA] Limit walker steps.

Wed Aug 26 15:52:57 PDT 2020

asbirlea accepted this revision.
asbirlea added inline comments.
This revision is now accepted and ready to land.

================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1825
+      DomAccess = MSSA.getSkipSelfWalker()->getClobberingMemoryAccess(
+          CurrentUD, DefLoc, ClobberSteps);
       if (MSSA.isLiveOnEntryDef(DomAccess))
----------------
fhahn wrote:
> asbirlea wrote:
> > fhahn wrote:
> > > asbirlea wrote:
> > > > use: /*WalkerUpwardsLimit=*/0
> > > > remove ClobberSteps.
> > > > 
> > > > Why use 0?
> > > The argument is taken by reference, and the walker decrements it as it goes along I think. I meant to use 1 as limit (single step), added a comment as to why. I think the behavior is actually the same whether 0 or 1 is passed, as I think if it is 0 initially it gets bumped to 1 internally.
> > Yes, I missed it needs a reference there.
> > 
> > You're right that 0 and 1 mean the same, I should have phrased the question clearer with what I meant.
> > If the cap you set is 0, this isn't getting more information than `getDefiningAccess`. The `getClobberingAccess` call should never reach the `tryOptimizePhi` call for the 0/1 caps.
> > If the intention is to deal with phi translation (as the comment says), you need a cap of 2+, and it will be dependent on the number of defs per block if it actually reaches the Phi.
> Thanks for clarifying. Currently it appears that actually using the clobber walker incurs too much compile-time overhead without too much gains. By doing the walk ourselves, we can stop traversing non-profitable paths earlier, as is done in D86487.
> 
> I removed the `getClobberingMemoryAccess` call. In the future, it might be good to extend the code here to optimize memory defs & uses if possible based on the checks we have to do anyways. It would be great to get some input on that and potentially additional tricks we can apply to MemorySSA, but that would probably best after the switch, once we got a decent amount of testing.
> 
> I also renamed the `Dom*` variable names, as `Earlier*` seems more appropriate and the dominance relation is not really important here.
sgtm, let's follow up on that.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86486/new/

https://reviews.llvm.org/D86486