[PATCH] D77736: [DSE] Lift post-dominance for objs not accessible in caller.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Thanks for this!

Just a few nits. Idea and implementation looks fine to me otherwise



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1488
+  SmallPtrSet<const Value *, 16> InvisibleToCallerBeforeRet;
+  // Keep track of all of the objects that are invisible to the caller
   // function returns.
----------------
s/the caller$/the caller after/


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1539
+            State.InvisibleToCallerBeforeRet.insert(&I);
+          if (!CapturesBeforeRet && !PointerMayBeCaptured(&I, true, false))
+            State.InvisibleToCallerAfterRet.insert(&I);
----------------
nit: can this just be nested in the above if, rather than repeating `!CapturesBeforeRet`?


================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-multipath.ll:139
 
+; The store in the entry block can be eliminated, because the it is overwritten
+; on all paths to the exit.
----------------
s/because the it/because it/ (similar below)


================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-multipath.ll:217
+; The store in the entry block cannot be eliminated. There's a path from the
+;  first store to the read in bb5, where the location is not overwritten.
 define void @alloca_3(i1 %c1) {
----------------
world's smallest ₙᵢₜ: please remove the extra space before 'first'


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77736