[llvm] [DSE] Apply initializes attribute to DSE (PR #107282)
Arthur Eubanks via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 30 11:19:11 PDT 2024
================
@@ -2232,8 +2366,11 @@ DSEState::eliminateDeadDefs(const MemoryLocationWrapper &KillingLocWrapper) {
}
MemoryDefWrapper DeadDefWrapper(
cast<MemoryDef>(DeadAccess),
- getLocForInst(cast<MemoryDef>(DeadAccess)->getMemoryInst()));
- MemoryLocationWrapper &DeadLocWrapper = *DeadDefWrapper.DefinedLocation;
+ getLocForInst(cast<MemoryDef>(DeadAccess)->getMemoryInst(),
+ /*ConsiderInitializesAttr=*/false));
+ assert(DeadDefWrapper.DefinedLocations.size() == 1);
+ MemoryLocationWrapper &DeadLocWrapper =
+ DeadDefWrapper.DefinedLocations.front();
----------------
aeubanks wrote:
after staring at this a bit, I believe this preexisting code is conflating two things: it's assuming that if there is a memory location that `DeadDefWrapper` writes to that overlaps with `KillingLocWrapper`, it must have no other side effects and be deletable. this happens to be true for stores and libcalls like `strcpy` that are handled here, but is not necessarily true in general.
I think ideally we change `isRemovable` to be more accurate about arbitrary function calls, and check that here, but I'm ok with a TODO saying something like `TODO: this conflates the existence of a MemoryLocation with being able to delete the instruction. fix isRemovable() to consider calls with side effects that cannot be removed, e.g. calls with the initializes attribute, and remove getLocForInst(ConsiderInitializesAttr = false) workaround
https://github.com/llvm/llvm-project/pull/107282
More information about the llvm-commits
mailing list