[PATCH] D73763: [DSE] Lift post-dominance restriction.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

I don't think MemorySSA should be adding that generic intrinsic. If we need such a hack at least contain it to within the pass, not an analysis used throughout the pass pipeline.
+ at george.burgess.iv for his thoughts on this too.



================
Comment at: llvm/include/llvm/IR/Intrinsics.td:1160
 
+def int_mssa_exit_read: Intrinsic<[], [], [IntrReadMem]>;
+
----------------
I wouldn't use this name. As far as I see, it has nothing to do with MSSA, it's just a generic intrinsic that models a read-only function call. 
This looks more akin to `int_donothing_mayread`.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1501
   DSEState(Function &F, AliasAnalysis &AA, MemorySSA &MSSA, DominatorTree &DT,
            PostDominatorTree &PDT, const TargetLibraryInfo &TLI)
       : F(F), AA(AA), MSSA(MSSA), DT(DT), PDT(PDT), TLI(TLI) {}
----------------
Cleanup PDT if it's not used anymore?


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1540
+            CI, nullptr, CI->getParent(), MemorySSA::End);
+        if (isa<MemoryUse>(NewMemAcc))
+          MSSAU.insertUse(cast<MemoryUse>(NewMemAcc), false);
----------------
If this intrinsic is read only, then assert a MemoryUse was created?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73763