[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