[PATCH] D133827: [DSE] Rewrite function memoryIsNotModified using MemorySSA
luxufan via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 14 02:19:36 PDT 2022
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
More information about the llvm-commits
mailing list