[all-commits] [llvm/llvm-project] 1f1145: [DSE] Use correct memory location for read clobber...

[Nikita Popov via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 1f1145006b32533484c9ebc0f45e241a02fe6c8b
      https://github.com/llvm/llvm-project/commit/1f1145006b32533484c9ebc0f45e241a02fe6c8b
  Author: Nikita Popov <nikita.ppv at gmail.com>
  Date:   2020-12-18 (Fri, 18 Dec 2020)

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

  Log Message:
  -----------
  [DSE] Use correct memory location for read clobber check

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

Differential Revision: https://reviews.llvm.org/D93523