[RS4GC] Handle special cases in unreachable code for memcpy/memmov

[mkazantsev]

Summary

The existing code doesn't expect dummy values (undef, poison, null-derived
constants etc) as arguments of these intrinsics. However, they can be there
in unreached code. Currently we fail trying to find base for them.

Handle these cases separately.

[apilipenko]

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1705

I suggest handling the undef case explicitly. Just return undef if the input is undef.

[mkazantsev]

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1705

Same goes for poison I guess.

[mkazantsev]

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1705

It's a no-go. Aside from undef and poison, here can be things like

i32 addrspace(1)* bitcast (i8 addrspace(1)* getelementptr inbounds (i8, i8 addrspace(1)* null, i64 16) to i32 addrspace(1)*)

etc. So the expression can be arbitrarily complex.

[mkazantsev]

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1705

General idea is that undef may not be the immediate value, but a part of any other expression. Therefore, there is no way to guarantee it's not involved somewhere. The assert is just over-conservative w.r.t. this situation.

[apilipenko]

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1705

I see your point.

Looks like RS4GC doesn't track liveness for constants. Instead, it assumes null base for them. See https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp#L537
I think we should do the same here to be consistent.

[mkazantsev]

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1705

No, we can't do it. Imagine you had smth like

i32 addrspace(1)* bitcast (i8 addrspace(1)* getelementptr inbounds (i8, i8 addrspace(1)* %base, i64 16) to i32 addrspace(1)*)

and then unreached code opt made it into

i32 addrspace(1)* bitcast (i8 addrspace(1)* getelementptr inbounds (i8, i8 addrspace(1)* %base, i64 undef) to i32 addrspace(1)*)

It's unreached code opts. We can't make any assumptions here.

[apilipenko]

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1705

In this case, the pointer should be in the live set.

[mkazantsev]

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1705

No it's not. Just run the patch's test and see.

[mkazantsev]

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1705

I'm wrong here, I was checking only null constant and that's why.

[apilipenko]

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1710

Undef and poison fall under the constant clause. I don't think that they need a special handling.

[mkazantsev]

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1710

Err, you are right.

[apilipenko]

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1706

I would add to the comment that taking null as a base is consistent with the handling in the main algorithm in findBaseDefiningValue.

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

if (IID == Intrinsic::experimental_deoptimize) {
       // intrinsic should know the corresponding base pointers.
       //
       // To make the base pointers available pass them explicitly as arguments:
       //   memcpy(dest_derived, source_derived, ...) =>
       //   memcpy(dest_base, dest_offset, source_base, source_offset, ...)
       auto &Context = Call->getContext();
       auto &DL = Call->getModule()->getDataLayout();
       auto GetBaseAndOffset = [&](Value *Derived) {
-        assert(PointerToBase.count(Derived));
         unsigned AddressSpace = Derived->getType()->getPointerAddressSpace();
         unsigned IntPtrSize = DL.getPointerSizeInBits(AddressSpace);
-        Value *Base = PointerToBase.find(Derived)->second;
+        auto It = PointerToBase.find(Derived);
+        Value *Base = It != PointerToBase.end() ? It->second : Derived;
         Value *Base_int = Builder.CreatePtrToInt(
             Base, Type::getIntNTy(Context, IntPtrSize));
         Value *Derived_int = Builder.CreatePtrToInt(
             Derived, Type::getIntNTy(Context, IntPtrSize));
         return std::make_pair(Base, Builder.CreateSub(Derived_int, Base_int));
       };
 
       auto *Dest = CallArgs[0];
       Value *DestBase, *DestOffset;
       std::tie(DestBase, DestOffset) = GetBaseAndOffset(Dest);

llvm/test/Transforms/RewriteStatepointsForGC/pr56493.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -passes=rewrite-statepoints-for-gc -S < %s | FileCheck %s
+
+; Make sure this doesn't crash.
+define void @test() gc "statepoint-example" personality i32* ()* @zot {
+; CHECK-LABEL: @test(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    [[STATEPOINT_TOKEN:%.*]] = call token (i64, i32, void (i32 addrspace(1)*, i64, i32 addrspace(1)*, i64, i64)*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidp1i32i64p1i32i64i64f(i64 2882400000, i32 0, void (i32 addrspace(1)*, i64, i32 addrspace(1)*, i64, i64)* elementtype(void (i32 addrspace(1)*, i64, i32 addrspace(1)*, i64, i64)) @__llvm_memcpy_element_unordered_atomic_safepoint_4, i32 5, i32 0, i32 addrspace(1)* undef, i64 undef, i32 addrspace(1)* bitcast (i8 addrspace(1)* getelementptr inbounds (i8, i8 addrspace(1)* null, i64 16) to i32 addrspace(1)*), i64 sub (i64 ptrtoint (i8 addrspace(1)* getelementptr inbounds (i8, i8 addrspace(1)* null, i64 16) to i64), i64 ptrtoint (i8 addrspace(1)* getelementptr inbounds (i8, i8 addrspace(1)* null, i64 16) to i64)), i64 undef, i32 0, i32 0) [ "deopt"() ]
+; CHECK-NEXT:    ret void
+;
+bb:
+  call void @llvm.memcpy.element.unordered.atomic.p1i32.p1i32.i64(i32 addrspace(1)* elementtype(i32) align 8 undef, i32 addrspace(1)* elementtype(i32) align 16 bitcast (i8 addrspace(1)* getelementptr inbounds (i8, i8 addrspace(1)* null, i64 16) to i32 addrspace(1)*), i64 undef, i32 4) #3 [ "deopt"() ]
+  ret void
+}
+
+declare i32* @zot()
+
+declare void @llvm.memcpy.element.unordered.atomic.p1i32.p1i32.i64(i32 addrspace(1)* nocapture writeonly, i32 addrspace(1)* nocapture readonly, i64, i32 immarg)