[PATCH] D93523: [DSE] Use correct memory location for read clobber check

[Nikita Popov via Phabricator via llvm-commits]

nikic created this revision.
nikic added reviewers: fhahn, asbirlea.
Herald added subscribers: george.burgess.iv, hiraditya, Prazek.
nikic requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

MSSA DSE starts at a killing store, finds an earlier store and then checks that the earlier store is not read along any paths (without being killed first). However, it uses the memory location of the killing store for that, not the earlier store that we're attempting to eliminate.

This has a number of problems:

- Mismatches between what BasicAA considers aliasing and what DSE considers an overwrite (even though both are correct in isolation) can result in miscompiles. This is PR48279, which D92045 <https://reviews.llvm.org/D92045> tries to fix in a different way. The problem is that we're using a location from a store that is potentially not executed and thus may be UB, in which case analysis results can be arbitrary.
- Metadata on the killing store may be used to determine aliasing, but there is no guarantee that the metadata is valid, as the specific killing store may not be executed. Using the metadata on the earlier store is valid (it is the store we're removing, so on any execution where its removal may be observed, it must be executed).
- The location is imprecise. For full overwrites the killing store will always have a location that is larger or equal than the earlier access location, so it's beneficial to use the earlier access location. This is not the case for partial overwrites, in which case either location might be smaller. There is some room for improvement here.

Using the earlier access location means that we can no longer cache which accesses are read for a given killing store, as we may be querying different locations. However, it turns out that simply dropping the cache has no notable impact on compile-time, so that seems fine: https://llvm-compile-time-tracker.com/compare.php?from=3d56644f18eefe30e353e7fae3cb5e5daf0a0ffb&to=9bdb4e5b085bc3d054c30f6256c344a14748a715&stat=instructions


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D93523

Files:
  llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
  llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-memintrinsics.ll
  llvm/test/Transforms/DeadStoreElimination/MSSA/out-of-bounds-stores.ll
  llvm/test/Transforms/DeadStoreElimination/MSSA/overlap.ll
  llvm/test/Transforms/DeadStoreElimination/MSSA/scoped-noalias.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D93523.312701.patch
Type: text/x-patch
Size: 12234 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201218/c5130add/attachment.bin>