This is an archive of the discontinued LLVM Phabricator instance.

[DSE] Don't DSE stores that subsequent memmove calls read from
ClosedPublic

Authored by sanjoy on Feb 16 2018, 5:15 PM.

Details

Summary

We used to remove the first memmove in cases like this:

memmove(p, p+2, 8);
memmove(p, p+2, 8);

which is incorrect. Fix this by changing isPossibleSelfRead to what was most
likely the intended behavior.

Historical note: the buggy code was added in https://reviews.llvm.org/rL120974
to address PR8728.

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy created this revision.Feb 16 2018, 5:15 PM
efriedma added inline comments.
test/Transforms/DeadStoreElimination/simple.ll
261 ↗(On Diff #134777)
rsmith added inline comments.Feb 16 2018, 6:40 PM
lib/Transforms/Scalar/DeadStoreElimination.cpp
513–514 ↗(On Diff #134777)

Change these to memmove so the second line isn't UB.

524 ↗(On Diff #134777)

Remove unused parameter DepWrite?

test/Transforms/DeadStoreElimination/simple.ll
261 ↗(On Diff #134777)

Gross. Until we figure out what we're going to do about that, perhaps we should just treat memcpy the same as memmove here.

sanjoy updated this revision to Diff 135136.Feb 20 2018, 1:33 PM
sanjoy marked an inline comment as done.
  • Relax memcpy handling

Reverted back to a more conservative memcpy handling.

lib/Transforms/Scalar/DeadStoreElimination.cpp
524 ↗(On Diff #134777)

Not relevant anymore since in this version of the patch I use DepWrite.

test/Transforms/DeadStoreElimination/simple.ll
261 ↗(On Diff #134777)

Wow, that's a huge omission from the langref (which specifically says that the source and destination cannot overlap).

Given that, the previous code makes more sense to me that it did initially.

This revision is now accepted and ready to land.Feb 20 2018, 1:57 PM
This revision was automatically updated to reflect the committed changes.