[PATCH] D79391: [DSE] Remove noop stores in MSSA.

[Zoe Carver via Phabricator via llvm-commits]

zoecarver marked an inline comment as done.
zoecarver added a comment.

@asbirlea thank you for the review! Sorry I didn't get to this sooner, I was super busy last week.

> Yup! 7%-15% is huge. I'm hoping this is due to the unnecessary processing I commented on, but won't know until tested with the changes. I'm pretty curios which part led to that big of a regression.

I'm pretty sure it's because I recursively walk *every* store's accesses. I suspect checking `Load->getPointerOperand() != Store->getOperand(1)` *first* will fix this. Running the stats again.



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1843
+
+      bool IsDead = LoadAccess == KillingDef->getDefiningAccess();
+      if (!IsDead) {
----------------
asbirlea wrote:
> I don't entirely understand the logic here, it would help a lot to add some comments to elaborate.
> 
> 
> What happens if `IsDead` is true? Seems this would be a good case to eliminate the store if the load/store are from/to the same address.
> 
> Next, check the condition `Load->getPointerOperand() != SI->getOperand(1)` first, and exit early if the equality condition is not met.
> 
> I see this roughly as:
> ```
> if (Load->getPointerOperand() == SI->getOperand(1)) {
>   if (LoadAccess == KillingDef->getDefiningAccess()) {
>     State.deleteDeadInstruction(SI);
>   } else {
>     //Note: LoadAccess may not be optimized, if it is the walker call is a no-op.
>     auto *LoadAccessOptimized = MSSAWalker->getClobberingMemoryAccess(MSSA.getMemoryAccess(Load));
>     if (MSSAWalker->getClobberingMemoryAccess(KillingDef) == LoadAccessOptimized) {
>       State.deleteDeadInstruction(SI);
>     }
>   }
> }
> ```
> 
> Please let me know if I misunderstood the intention here.
I've re-factored this a bit and added comments. It should be much clearer now. Let me know if not, and I can explain further. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79391