[PATCH] D51717: [EarlyCSEwMemorySSA] Fix failure (w/ expensive checks). Need to resetOptimizeUses for replaced instructions.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Thanks for this!

> This may need to be reconsidered and added under the VerifyMemorySSA bool, so we can check such failures in tests

SGTM.

I don't fully understand the failure this is meant to fix; question inline.



================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:607
       return;
+    if (VerifyMemorySSA)
+      MSSA->verifyMemorySSA();
----------------
Are these part of the fix? :)


================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:630
 
         MSSAUpdater->removeMemoryAccess(WI);
 
----------------
AFAICT, this `removeMemoryAccess` should be doing all of the cache invalidation that we need to do. It should boil down to "point all of the memory things pointing at `WI` (which, on the first iteration, is `MA`) to the closest dominating MemoryAccess".

This should implicitly reset all cached Def and Use optimizations. Uses, when we want to find the actual optimized access, will start walking from said closest dominating MemoryAccess. Defs will start walking from their defining access.

Is the reset causing problems (e.g. is it somehow causing a Use to be optimized over its actual clobber)? Or is the replacement not causing invalidation? Or am I missing something?


================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:916
+            MSSAUpdater->resetOptimizeUses(Inst);
           Inst->replaceAllUsesWith(V);
           Changed = true;
----------------
(Seeing this without a corresponding MSSA update is somewhat concerning, though I guess it's reasonable to assume that if the instruction is trivially simplified + isn't trivially dead, it's not a memory instruction?)


Repository:
  rL LLVM

https://reviews.llvm.org/D51717