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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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,
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133827