[DeadStoreElimination] Check function modref behavior before considering memory clobbered

[igor-laevsky]

Summary

Currently we assume that if function may write to memory and it has MRI_ModRef behaviour in relation to the requested memory location then it means that this function writes to the memory location. In fact it's possible that it only reads from it. For example we can achieve this by using readonly attribute on the function operand.

[igor-laevsky]

ping?

[• dberlin]

So
A. This added test still fails with this patch for me :)
B. I see this is only used in the case where the code is trying to eliminate redundant stores, so seems okay otherwise.

[igor-laevsky]

@dberlin
A: Sorry, I should have mentioned that this change depends the alias analysis change: https://reviews.llvm.org/D29989
B: Yes, this function specifically checks for memory modifications, so it shouldn't be used to determine if store is dead.

[sanjoy]

test/Transforms/DeadStoreElimination/operand-bundles.ll

48 ↗

(On Diff #88565)

Looks like today -basicaa -dse does not elide the second store in

declare void @foo()
declare void @bar(i8*)

define void @test4(i8* %ptr) {
  store i8 0, i8* %ptr, align 4
  call void @foo() readonly
  store i8 0, i8* %ptr, align 4
  call void @bar(i8* nocapture %ptr)
  ret void
}

Will it be able to do that with your changes? If so, adding that as a test case will be nice.

[reames]

The actual code change looks obviously correct.

LGTM once the AA change lands.

test/Transforms/DeadStoreElimination/operand-bundles.ll

48 ↗

(On Diff #88565)

It doesn't look like we have the framework for this in DSE which is unfortunate. Oddly enough earlycse probably will get this example.

llvm/trunk/lib/Transforms/Scalar/DeadStoreElimination.cpp

if (isFirstBlock) {
       // It's not SecondBB or (in case of a loop) the second visit of SecondBB.
       // In this case we also have to look at instructions after SI.
       EI = B->end();
     }
     for (; BI != EI; ++BI) {
       Instruction *I = &*BI;
       if (I->mayWriteToMemory() && I != SecondI) {
         auto Res = AA->getModRefInfo(I, MemLoc);
-        if (Res != MRI_NoModRef)
+        if (Res & MRI_Mod)
           return false;
       }
     }
     if (B != FirstBB) {
       assert(B != &FirstBB->getParent()->getEntryBlock() &&
           "Should not hit the entry block because SI must be dominated by LI");
       for (auto PredI = pred_begin(B), PE = pred_end(B); PredI != PE; ++PredI) {
         if (!Visited.insert(*PredI).second)

llvm/trunk/test/Transforms/DeadStoreElimination/operand-bundles.ll

define void @test3() {
   ; Verify that this first store is not considered killed by the second one
   ; since it could be observed from the deopt continuation.
   ; CHECK: store i64 1, i64* %s
   store i64 1, i64* %s
   call void @foo() [ "deopt"(i64* %s) ]
   store i64 0, i64* %s
   ret void
 }
+
+declare noalias i8* @calloc(i64, i64)
+
+define void @test4() {
+; CHECK-LABEL: @test4
+  %local_obj = call i8* @calloc(i64 1, i64 4)
+  call void @foo() ["deopt" (i8* %local_obj)]
+  store i8 0, i8* %local_obj, align 4
+  ; CHECK-NOT: store i8 0, i8* %local_obj, align 4
+  call void @bar(i8* nocapture %local_obj)
+  ret void
+}