[PATCH] D133827: [DSE] Rewrite function memoryIsNotModified using MemorySSA

[luxufan via Phabricator via llvm-commits]

StephenFan added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:386
 /// Precondition: Second instruction must be dominated by the first
 /// instruction.
+static bool memoryIsNotModifiedBetween(MemorySSA &MSSA, Instruction *FirstI,
----------------
nikic wrote:
> Do I understand correctly that another precondition of this code is now that FirstI must mod SecondI? Otherwise the walk may go past `FirstI` and an incorrect modification would be reported?
> 
> I think it would be better to do a domination check, as in https://github.com/llvm/llvm-project/blob/419580c699481de40f1e819c396fe07a63885b43/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp#L369.
> 
> We should also assert that SecondI is a MemoryDef, for MemoryUses different logic is required.
> 
> I'm also slightly concerned that this may produce incorrect results for the loop varying case if SecondI is a call, as MSSA has a known issue in this area.
Thanks for reviewing! Would you mind posting a test case that can reflect the known issue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133827