freeze does not change the constant property

[danilaml]

Summary

Traverse through freeze when checking if base is constant

[skatkov]

lgtm

[reames]

Non blocking comment..

freeze(undef) being equally null or base as undef seems questionable to me. I think in practice it might work out okay, but there's no guarantee that the value chosen for undef isn't 0 when frozen, and 1 when not. You might want to step back and take a deeper look at the problem you're solving here.

[danilaml]

In D127744#3581771, @reames wrote:

Non blocking comment..

freeze(undef) being equally null or base as undef seems questionable to me. I think in practice it might work out okay, but there's no guarantee that the value chosen for undef isn't 0 when frozen, and 1 when not. You might want to step back and take a deeper look at the problem you're solving here.

Hm, I don't see how the value eventually chosen for freeze/undef factor into this verifier check. It doesn't try to guess/resolve undef/poison in any way, so it should be conservative here by only treating them as some constants (isa<Constant>(V) is true for them). In other words, if the value is already exclusively null (=> not undef or poison), "freezing" it won't change that property (so it's safe to look through it). Similarly, if it's already a constant (including undef or poison). Or maybe I'm approaching it wrong?

llvm/lib/IR/SafepointIRVerifier.cpp

if (const auto *SI = dyn_cast<SelectInst>(V)) {
       continue;
     }
     if (const auto *GCRelocate = dyn_cast<GCRelocateInst>(V)) {
       // GCRelocates do not change null-ness or constant-ness of the value.
       // So we can continue with derived pointer this instruction relocates.
       Worklist.push_back(GCRelocate->getDerivedPtr());
       continue;
     }
+    if (const auto *FI = dyn_cast<FreezeInst>(V)) {
+      // Freeze does not change null-ness or constant-ness of the value.
+      Worklist.push_back(FI->getOperand(0));
+      continue;
+    }
     if (isa<Constant>(V)) {
       // We found at least one base pointer which is non-null, so this derived
       // pointer is not exclusively derived from null.
       if (V != Constant::getNullValue(V->getType()))
         isExclusivelyDerivedFromNull = false;
       // Continue processing the remaining values to make sure it's exclusively
       // constant.
       continue;

llvm/test/SafepointIRVerifier/hidden-constant-base.ll

-; RUN: opt -safepoint-ir-verifier-print-only -verify-safepoint-ir -S %s 2>&1 | FileCheck %s
+; RUN: opt -safepoint-ir-verifier-print-only -passes='verify<safepoint-ir>' -S %s 2>&1 | FileCheck %s
 
 ; CHECK-LABEL: Verifying gc pointers in function: test
-; CHECK: No illegal uses found by SafepointIRVerifier in: test
-define i8 addrspace(1)* @test(i1 %arg) gc "statepoint-example" {
+; CHECK: No illegal uses found by SafepointIRVerifier in: test_gc_relocate
+define i8 addrspace(1)* @test_gc_relocate(i1 %arg) gc "statepoint-example" {
 bb:
   br label %bb2
 
 bb2:                                              ; preds = %bb8, %bb
   %tmp = phi i8 addrspace(1)* [ %tmp5.relocated, %bb8 ], [ null, %bb ]
   %statepoint_token = call token (i64, i32, void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 2882400000, i32 0, void ()* elementtype(void ()) @widget, i32 0, i32 0, i32 0, i32 0) [ "deopt"() ]
   br label %bb4
 
 bb4:                                              ; preds = %bb8, %bb2
   %tmp5 = phi i8 addrspace(1)* [ %tmp5.relocated, %bb8 ], [ %tmp, %bb2 ]
   %statepoint_token1 = call token (i64, i32, i1 ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_i1f(i64 2882400000, i32 0, i1 ()* elementtype(i1 ()) @baz, i32 0, i32 0, i32 0, i32 0) [ "deopt"(i8 addrspace(1)* %tmp5), "gc-live"(i8 addrspace(1)* %tmp5) ]
   %tmp62 = call i1 @llvm.experimental.gc.result.i1(token %statepoint_token1)
   %tmp5.relocated = call coldcc i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(token %statepoint_token1, i32 0, i32 0) ; (%tmp5, %tmp5)
   br i1 %tmp62, label %bb8, label %bb6
 
 bb6:                                              ; preds = %bb4
   ret i8 addrspace(1)* null
 
 bb8:                                              ; preds = %bb4
   br i1 %arg, label %bb4, label %bb2
 }
 
+; CHECK-LABEL: Verifying gc pointers in function: test
+; CHECK: No illegal uses found by SafepointIRVerifier in: test_freeze_inst
+define void @test_freeze_inst(i32* %ptr) gc "statepoint-example" {
+entry:
+  br label %loop
+
+loop:
+  %vptr = phi i8 addrspace(1)* [ %fptr, %loop ], [ undef, %entry ]
+  %fptr = freeze i8 addrspace(1)* %vptr
+  %statepoint_token = call token (i64, i32, void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 0, i32 0, void ()* elementtype(void ()) undef, i32 0, i32 0, i32 0, i32 0) [ "deopt"(i8 addrspace(1)* %fptr), "gc-live"(i8 addrspace(1)* %fptr) ]
+  %fptr.relocated = call coldcc i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(token %statepoint_token, i32 0, i32 0) ; (%fptr, %fptr)
+  br label %loop
+}
+
 declare void @widget()
 declare i1 @baz()
 
 declare token @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 immarg, i32 immarg, void ()*, i32 immarg, i32 immarg, ...)
 declare token @llvm.experimental.gc.statepoint.p0f_i1f(i64 immarg, i32 immarg, i1 ()*, i32 immarg, i32 immarg, ...)
 declare i1 @llvm.experimental.gc.result.i1(token)
 declare i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(token, i32 immarg, i32 immarg)