[CodeGen] Fix isGCValue utility for statepoint lowering

[skatkov]

Summary

isGCValue should detect whether the deopt value is a GC pointer.
Currently it checks by finding the value in SI.Bases and SI.Ptrs.
However these dsata structures contains only those values which
has corresponding gc.relocate call. So we can miss GC value is it
does not have gc.relocate call (dead after the call).

At the same time specification requires for all deopt values which
can be modified by GC to be listed in gc values.
So we can check whether deopt value is listed in gc values.

[dantrushin]

llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp

486

Well, this makes me to ask a question, see below (at lines 497 and 530)...

497

Is it correct that we process SI.Bases and SI.Ptrs here, not SI.GCArgs?

530

And here we lower not all GCArgs, but relocated pointers only....

[reames]

Er, I think the specification simplify needs changed here. As we've discussed offline, it's impossible to enforce having a gc pointer in the deopt list always be in the gc list (in any form). This was a flaw in the specification from the begging and we should fix the docs, not try to hack up the code to preserve it a bit longer.

The key example of why this can't work:
%p1 = bitcast i8* %p to i8*
statepoint [gc = (%p1)], [deopt = (%p1)]

The optimizer is allowed to replace either use (or both) of %p1 with %p. If it updates only one of the two (entirely legal), the two sets do not overlap.

If we need to enforce a property for GC values within deopt list, we should directly use the type of the pointer in question. Anything else appears unsound.

[skatkov]

In D77036#1950340, @reames wrote:

Er, I think the specification simplify needs changed here. As we've discussed offline, it's impossible to enforce having a gc pointer in the deopt list always be in the gc list (in any form). This was a flaw in the specification from the begging and we should fix the docs, not try to hack up the code to preserve it a bit longer.

The key example of why this can't work:
%p1 = bitcast i8* %p to i8*
statepoint [gc = (%p1)], [deopt = (%p1)]

The optimizer is allowed to replace either use (or both) of %p1 with %p. If it updates only one of the two (entirely legal), the two sets do not overlap.

If we need to enforce a property for GC values within deopt list, we should directly use the type of the pointer in question. Anything else appears unsound.

ok, then I will prepare a patch which eliminates the questionable requirement from spec.

In terms of fixing this one I see two ways to resolve it:

1. Use the same isHandledGCPointerType utility as RewriteStatepointsForGC for detection of GC pointer
2. Conservatively suggest any pointer to be a GC pointer.

What way do you prefer?

llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp

497

It is fine due to it is only optimization and is not required for correctness.

530

That is also fine. Bases and Ptrs are build from GCArgs by elimination of the same values, so it is ok to lower only unique values.

[skatkov]

Please take a loot at https://reviews.llvm.org/D77122 and https://reviews.llvm.org/D77130.

[skatkov]

in favor of https://reviews.llvm.org/D77130

llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp

#endif
   // particular, if the callee spills callee preserved registers we may not
   // be able to find a value placed in that register during the call.  This is
   // fine for live-out, but not for live-through.  If we were willing to make
   // assumptions about the code generator producing the callee, we could
   // potentially allow live-through values in callee saved registers.
   const bool LiveInDeopt =
     SI.StatepointFlags & (uint64_t)StatepointFlags::DeoptLiveIn;
 
-  auto isGCValue =[&](const Value *V) {
-    return is_contained(SI.Ptrs, V) || is_contained(SI.Bases, V);
-  };
+  auto isGCValue = [&](const Value *V) { return is_contained(SI.GCArgs, V); };
 
   // Before we actually start lowering (and allocating spill slots for values),
   // reserve any stack slots which we judge to be profitable to reuse for a
   // particular value.  This is purely an optimization over the code below and
   // doesn't change semantics at all.  It is important for performance that we
   // reserve slots for both deopt and gc values before lowering either.
   for (const Value *V : SI.DeoptState) {
     if (!LiveInDeopt || isGCValue(V))
       reservePreviousStackSlotForValue(V, Builder);
   }
   for (unsigned i = 0; i < SI.Bases.size(); ++i) {
     reservePreviousStackSlotForValue(SI.Bases[i], Builder);
     reservePreviousStackSlotForValue(SI.Ptrs[i], Builder);
   }
 
   // First, prefix the list with the number of unique values to be
   // lowered.  Note that this is the number of *Values* not the
   // number of SDValues required to lower them.
   const int NumVMSArgs = SI.DeoptState.size();
for (const Value *V : SI.DeoptState) {
     lowerIncomingStatepointValue(Incoming, LiveInValue, Ops, MemRefs, Builder);
   }
 
   // Finally, go ahead and lower all the gc arguments.  There's no prefixed
   // length for this one.  After lowering, we'll have the base and pointer
   // arrays interwoven with each (lowered) base pointer immediately followed by
   // it's (lowered) derived pointer.  i.e
   // (base[0], ptr[0], base[1], ptr[1], ...)
   for (unsigned i = 0; i < SI.Bases.size(); ++i) {
     const Value *Base = SI.Bases[i];
     lowerIncomingStatepointValue(Builder.getValue(Base), /*LiveInOnly*/ false,
                                  Ops, MemRefs, Builder);
 
     const Value *Ptr = SI.Ptrs[i];
     lowerIncomingStatepointValue(Builder.getValue(Ptr), /*LiveInOnly*/ false,
                                  Ops, MemRefs, Builder);
   }

llvm/test/CodeGen/X86/statepoint-live-in.ll

 ; CHECK-NEXT:    retq
 entry:
   %a = alloca i32
   store i32 %v, i32* %a
   %statepoint_token1 = call token (i64, i32, void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 2882400000, i32 0, void ()* @bar, i32 0, i32 2, i64 0, i64 3, i32* %a, i32* %a, i32* %a)
   ret void
 }
 
+; Demonstrate address of a function (w/ spilling due to it is gc value)
+define void @addr_func2() gc "statepoint-example" {
+; CHECK-LABEL: addr_func2:
+; CHECK:       ## %bb.0: ## %entry
+; CHECK-NEXT:    pushq %rax
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
+; CHECK-NEXT:    movq _bar@{{.*}}(%rip), %rax
+; CHECK-NEXT:    movq    %rax, (%rsp)
+; CHECK-NEXT:    callq _bar
+; CHECK-NEXT:  Ltmp17:
+; CHECK-NEXT:    popq %rax
+; CHECK-NEXT:    retq
+entry:
+  %statepoint_token1 = call token (i64, i32, void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 2882400000, i32 0, void ()* @bar, i32 0, i32 2, i64 0, i64 2, void ()* @bar, void ()* @bar, void ()* @bar)
+  ret void
+}
 
 ; CHECK: Ltmp0-_test1
 ; CHECK:      .byte 1
 ; CHECK-NEXT:   .byte   0
 ; CHECK-NEXT: .short 4
 ; CHECK-NEXT: .short  5
 ; CHECK-NEXT:   .short  0
 ; CHECK-NEXT: .long 0