[DeadStoreElimination] Handle null accessing

[ChuanqiXu]

Summary

After we landed https://reviews.llvm.org/D127383, the compiler may crash due to it tries to access a may-null value. This is because https://github.com/llvm/llvm-project/blob/051738b08cf5e39fd274dd379147d1c19e2b5b20/llvm/lib/Analysis/MemoryLocation.cpp#L121-L122 may return null after we land D127383.

This should be a noop before D127383 landed.

[ChuanqiXu]

@nikic Are you happy to land this and D130153? Sorry I'm hurry since that I want to handle TLS problem in coroutines in clang15 and clang15 is going to be branched in 7.26

[ChuanqiXu]

Add tests

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp

1927

Since there is a comment. I am not sure if this is a good fix.

[nikic]

I think both this change and D130153 are fine in principle, but I am concerned about the larger issue this exposes. Code has been making (historically quite reasonable) assumptions about how certain intrinsics work, for example that debug intrinsics don't access memory. These assumptions are still correct, but LLVM doesn't know this anymore, because it thinks that debug intrinsics, or memset, etc can read the thread ID now (or in this case, it's one step further, in that we lose the information that memset can only write to its argument, because we don't track complex scenarios like "can only write argument memory, but read other memory").

I guess if the number of places that make assumptions are limited, it may be okay to add such workarounds, but it's rather unfortunate. I also suspect that this can manifest in less obvious ways, e.g. because debug invariance in some places may depend on debug intrinsics not accessing memory.

[ChuanqiXu]

In D130155#3675616, @nikic wrote:

I think both this change and D130153 are fine in principle, but I am concerned about the larger issue this exposes. Code has been making (historically quite reasonable) assumptions about how certain intrinsics work, for example that debug intrinsics don't access memory. These assumptions are still correct, but LLVM doesn't know this anymore, because it thinks that debug intrinsics, or memset, etc can read the thread ID now (or in this case, it's one step further, in that we lose the information that memset can only write to its argument, because we don't track complex scenarios like "can only write argument memory, but read other memory").

I guess if the number of places that make assumptions are limited, it may be okay to add such workarounds, but it's rather unfortunate. I also suspect that this can manifest in less obvious ways, e.g. because debug invariance in some places may depend on debug intrinsics not accessing memory.

The key point here is about the numbers of the assumptions. And if we break an assumption, we should meet a compilation crash immediately. So I think the scope of the problem could be tested. I've tested our internal workloads and folly (The largest open sourced user of coroutines I know). So it looks like the number is limited from the experiment.

[ChuanqiXu]

@nikic would you mind landing this? Since 15.x branch is going to be branched and the thread identification problem is really important to users of coroutines, I want to have a workaround for it. And there is more than one month long testing time so that we could revert it if we find anything bad.

[nikic]

In D130155#3678941, @ChuanqiXu wrote:

@nikic would you mind landing this? Since 15.x branch is going to be branched and the thread identification problem is really important to users of coroutines, I want to have a workaround for it. And there is more than one month long testing time so that we could revert it if we find anything bad.

Would it be "good enough" to mark coro.tls.wrapper as reading memory for LLVM 15, if we're adding a temporary intrinsic anyway? Or is it very important that __attribute__((const)) functions also get "fixed"?

[ChuanqiXu]

In D130155#3678987, @nikic wrote:

In D130155#3678941, @ChuanqiXu wrote:

@nikic would you mind landing this? Since 15.x branch is going to be branched and the thread identification problem is really important to users of coroutines, I want to have a workaround for it. And there is more than one month long testing time so that we could revert it if we find anything bad.

Would it be "good enough" to mark coro.tls.wrapper as reading memory for LLVM 15, if we're adding a temporary intrinsic anyway?

Do you mean to mark coro.tls.wrapper as readonly?

Or is it very important that __attribute__((const)) functions also get "fixed"?

Yeah, it is important. Many users need to call pthread_self() or getid() to get the current the thread.

And this should be fixed/workarounded in D127383. Do you mean to not land D130142 in 15.x? From my understanding, if we're concerning it breaks some assumptions, it should be easy to test. And if we're concerning about the impact to the IR model, it depends on whether we agree the consensus we made in previous discussions.

[ChuanqiXu]

@nikic I guess we could try to land this one since 15.x is branched and we have enough time to test.

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp

for (auto *Def : MemDefs) {
         continue;
 
       Instruction *UpperInst = UpperDef->getMemoryInst();
       auto IsRedundantStore = [&]() {
         if (DefInst->isIdenticalTo(UpperInst))
           return true;
         if (auto *MemSetI = dyn_cast<MemSetInst>(UpperInst)) {
           if (auto *SI = dyn_cast<StoreInst>(DefInst)) {
-            // MemSetInst must have a write location.
-            MemoryLocation UpperLoc = *getLocForWrite(UpperInst);
+            Optional<MemoryLocation> UpperLoc = getLocForWrite(UpperInst);
+            if (!UpperLoc)
+              return false;
+
             int64_t InstWriteOffset = 0;
             int64_t DepWriteOffset = 0;
-            auto OR = isOverwrite(UpperInst, DefInst, UpperLoc, *MaybeDefLoc,
+            auto OR = isOverwrite(UpperInst, DefInst, *UpperLoc, *MaybeDefLoc,
                                   InstWriteOffset, DepWriteOffset);
             Value *StoredByte = isBytewiseValue(SI->getValueOperand(), DL);
             return StoredByte && StoredByte == MemSetI->getOperand(1) &&
                    OR == OW_Complete;
           }
         }
         return false;
       };

llvm/test/Transforms/DeadStoreElimination/memset-presplitcoroutine.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -dse -S %s | FileCheck %s
+
+declare i8* @_Znwm() local_unnamed_addr #0
+
+; Function Attrs: argmemonly nounwind willreturn writeonly
+declare void @llvm.memset.p0i8.i64(i8* nocapture writeonly, i8, i64, i1 immarg) #1
+
+define i8* @test2(i1 %c, i64 %N) presplitcoroutine {
+; CHECK-LABEL: @test2(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br i1 [[C:%.*]], label [[COND_TRUE_I_I_I:%.*]], label [[COND_END_I_I_I:%.*]]
+; CHECK:       cond.true.i.i.i:
+; CHECK-NEXT:    ret i8* null
+; CHECK:       cond.end.i.i.i:
+; CHECK-NEXT:    [[ALLOC:%.*]] = tail call noalias nonnull i8* @_Znam() #[[ATTR3:[0-9]+]]
+; CHECK-NEXT:    [[ALLOC_BC:%.*]] = bitcast i8* [[ALLOC]] to i64*
+; CHECK-NEXT:    tail call void @llvm.memset.p0i8.i64(i8* nonnull align 8 [[ALLOC]], i8 0, i64 [[N:%.*]], i1 false) #[[ATTR4:[0-9]+]]
+; CHECK-NEXT:    store i64 0, i64* [[ALLOC_BC]], align 8
+; CHECK-NEXT:    ret i8* [[ALLOC]]
+;
+entry:
+  br i1 %c, label %cond.true.i.i.i, label %cond.end.i.i.i
+
+cond.true.i.i.i:                                  ; preds = %entry
+  ret i8* null
+
+cond.end.i.i.i:                                   ; preds = %entry
+  %alloc = tail call noalias nonnull i8* @_Znam() #2
+  %alloc.bc = bitcast i8* %alloc to i64*
+  tail call void @llvm.memset.p0i8.i64(i8* nonnull align 8 %alloc, i8 0, i64 %N, i1 false) #1
+  store i64 0, i64* %alloc.bc, align 8
+  ret i8* %alloc
+}
+
+declare i8* @_Znam()
+
+attributes #0 = { "use-soft-float"="false" }
+attributes #1 = { argmemonly nounwind willreturn writeonly }
+attributes #2 = { builtin nounwind }