Bug fix "GC relocate is incorrectly tied to the statepoint"

[dbakunevich]

Summary

Make Verifier recognize undef tokens as correct IR

Undef tokens may appear in unreached code as result of RAUW of some optimization,
and it should not be considered as bad IR.

Link to bug: https://github.com/llvm/llvm-project/issues/56267

[mkazantsev]

llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp

1202

This needs a test.

llvm/lib/IR/IntrinsicInst.cpp

701

remove

[apilipenko]

llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp

173

Assert isa<GCStatepointInst, UndefValue>(Statepoint)

1206

Assert isa<GCStatepointInst, UndefValue>(SI)

1235

Assert isa<GCStatepointInst, UndefValue>(SI)

llvm/lib/IR/Verifier.cpp

5159–5160

isa<GCStatepointInst, UndefValue>(Token)

[dbakunevich]

As part of this task, I could not use the

isa<Type_1,Type_2>(Val)

construct, because there was an error when building llvm.
Throughout llvm, the construction

isa<Type1>(Val) || isa<Type_2>(Val)

is often used. Therefore, I propose to solve the full transition problem with

isa<>() || isa<>()

to

isa<,>()

as a separate NFC patch

[mkazantsev]

llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp

173

Not sure if this assert is needed at all, because it will naturally be asserted by cast below.

llvm/lib/IR/IntrinsicInst.cpp

719–724

auto*

721

Remove const_cast

llvm/test/Transforms/InstCombine/gc.relocate-verify.ll

12

unreached

[mkazantsev]

Pls change patch description into smth more verbose, like

Make Verifier recognize undef tokens as correct IR

Undef tokens may appear in unreached code as result of RAUW of some optimization,
and it should not be considered as bad IR.

[mkazantsev]

LGTM w/ description fix.

llvm/include/llvm/IR/IntrinsicInst.h

public:
   /// This includes relocates which are on the unwinding path.
   bool isTiedToInvoke() const {
     const Value *Token = getArgOperand(0);
 
     return isa<LandingPadInst>(Token) || isa<InvokeInst>(Token);
   }
 
   /// The statepoint with which this gc.relocate is associated.
-  const GCStatepointInst *getStatepoint() const;
+  const Value *getStatepoint() const;
 };
 
 /// Represents calls to the gc.relocate intrinsic.
 class GCRelocateInst : public GCProjectionInst {
 public:
   static bool classof(const IntrinsicInst *I) {
     return I->getIntrinsicID() == Intrinsic::experimental_gc_relocate;
   }

llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp

static Optional<int> findPreviousSpillSlot(const Value *Val,
                                            SelectionDAGBuilder &Builder,
                                            int LookUpDepth) {
   // Can not look any further - give up now
   if (LookUpDepth <= 0)
     return None;
 
   // Spill location is known for gc relocates
   if (const auto *Relocate = dyn_cast<GCRelocateInst>(Val)) {
-    const auto &RelocationMap =
-        Builder.FuncInfo.StatepointRelocationMaps[Relocate->getStatepoint()];
+    const Value *Statepoint = Relocate->getStatepoint();
+    assert((isa<GCStatepointInst>(Statepoint) || isa<UndefValue>(Statepoint)) &&
+           "GetStatepoint must return one of two types");
+    if (isa<UndefValue>(Statepoint))
+      return None;
+
+    const auto &RelocationMap = Builder.FuncInfo.StatepointRelocationMaps
+                                    [cast<GCStatepointInst>(Statepoint)];
 
     auto It = RelocationMap.find(Relocate);
     if (It == RelocationMap.end())
       return None;
 
     auto &Record = It->second;
     if (Record.type != RecordType::Spill)
       return None;

 
 void SelectionDAGBuilder::LowerCallSiteWithDeoptBundle(
     const CallBase *Call, SDValue Callee, const BasicBlock *EHPadBB) {
   LowerCallSiteWithDeoptBundleImpl(Call, Callee, EHPadBB,
                                    /* VarArgDisallowed = */ false,
                                    /* ForceVoidReturnTy  = */ false);
 }
 
 void SelectionDAGBuilder::visitGCResult(const GCResultInst &CI) {
   // The result value of the gc_result is simply the result of the actual
   // call.  We've already emitted this, so just grab the value.
-  const GCStatepointInst *SI = CI.getStatepoint();
+  const Value *SI = CI.getStatepoint();
+  assert((isa<GCStatepointInst>(SI) || isa<UndefValue>(SI)) &&
+         "GetStatepoint must return one of two types");
+  if (isa<UndefValue>(SI))
+    return;
 
-  if (SI->getParent() == CI.getParent()) {
+  if (cast<GCStatepointInst>(SI)->getParent() == CI.getParent()) {
     setValue(&CI, getValue(SI));
     return;
   }
   // Statepoint is in different basic block so we should have stored call
   // result in a virtual register.
   // We can not use default getValue() functionality to copy value from this
   // register because statepoint and actual call return types can be
   // different, and getValue() will use CopyFromReg of the wrong type,
   // which is always i32 in our case.
   Type *RetTy = CI.getType();
   SDValue CopyFromReg = getCopyFromRegs(SI, RetTy);
   
   assert(CopyFromReg.getNode());
   setValue(&CI, CopyFromReg);
 }
 
 void SelectionDAGBuilder::visitGCRelocate(const GCRelocateInst &Relocate) {
 #ifndef NDEBUG
   // Consistency check
   // We skip this check for relocates not in the same basic block as their
   // statepoint. It would be too expensive to preserve validation info through
   // different basic blocks.
-  if (Relocate.getStatepoint()->getParent() == Relocate.getParent())
+  const Value *Statepoint = Relocate.getStatepoint();
+  assert((isa<GCStatepointInst>(Statepoint) || isa<UndefValue>(Statepoint)) &&
+         "GetStatepoint must return one of two types");
+  if (isa<UndefValue>(Statepoint))
+    return;
+
+  if (cast<GCStatepointInst>(Statepoint)->getParent() == Relocate.getParent())
     StatepointLowering.relocCallVisited(Relocate);
 
   auto *Ty = Relocate.getType()->getScalarType();
   if (auto IsManaged = GFI->getStrategy().isGCManagedPointer(Ty))
     assert(*IsManaged && "Non gc managed pointer relocated!");
 #endif
 
   const Value *DerivedPtr = Relocate.getDerivedPtr();
   auto &RelocationMap =
-    FuncInfo.StatepointRelocationMaps[Relocate.getStatepoint()];
+      FuncInfo.StatepointRelocationMaps[cast<GCStatepointInst>(Statepoint)];
   auto SlotIt = RelocationMap.find(&Relocate);
   assert(SlotIt != RelocationMap.end() && "Relocating not lowered gc value");
   const RecordType &Record = SlotIt->second;
 
   // If relocation was done via virtual register..
   if (Record.type == RecordType::SDValueNode) {
-    assert(Relocate.getStatepoint()->getParent() == Relocate.getParent() &&
+    assert(cast<GCStatepointInst>(Statepoint)->getParent() ==
+               Relocate.getParent() &&
            "Nonlocal gc.relocate mapped via SDValue");
     SDValue SDV = StatepointLowering.getLocation(getValue(DerivedPtr));
     assert(SDV.getNode() && "empty SDValue");
     setValue(&Relocate, SDV);
     return;
   }
   if (Record.type == RecordType::VReg) {
     Register InReg = Record.payload.Reg;

llvm/lib/IR/IntrinsicInst.cpp

 
 unsigned BinaryOpIntrinsic::getNoWrapKind() const {
   if (isSigned())
     return OverflowingBinaryOperator::NoSignedWrap;
   else
     return OverflowingBinaryOperator::NoUnsignedWrap;
 }
 
-const GCStatepointInst *GCProjectionInst::getStatepoint() const {
+const Value *GCProjectionInst::getStatepoint() const {
   const Value *Token = getArgOperand(0);
+  if (isa<UndefValue>(Token))
+    return Token;
 
   // This takes care both of relocates for call statepoints and relocates
   // on normal path of invoke statepoint.
   if (!isa<LandingPadInst>(Token))
     return cast<GCStatepointInst>(Token);
 
   // This relocate is on exceptional path of an invoke statepoint
   const BasicBlock *InvokeBB =
     cast<Instruction>(Token)->getParent()->getUniquePredecessor();
 
   assert(InvokeBB && "safepoints should have unique landingpads");
   assert(InvokeBB->getTerminator() &&
          "safepoint block should be well formed");
 
   return cast<GCStatepointInst>(InvokeBB->getTerminator());
 }
 
 Value *GCRelocateInst::getBasePtr() const {
-  if (auto Opt = getStatepoint()->getOperandBundle(LLVMContext::OB_gc_live))
+  auto Statepoint = getStatepoint();
+  if (isa<UndefValue>(Statepoint))
+    return UndefValue::get(Statepoint->getType());
+
+  auto *GCInst = cast<GCStatepointInst>(Statepoint);
+  if (auto Opt = GCInst->getOperandBundle(LLVMContext::OB_gc_live))
     return *(Opt->Inputs.begin() + getBasePtrIndex());
-  return *(getStatepoint()->arg_begin() + getBasePtrIndex());
+  return *(GCInst->arg_begin() + getBasePtrIndex());
 }
 
 Value *GCRelocateInst::getDerivedPtr() const {
-  if (auto Opt = getStatepoint()->getOperandBundle(LLVMContext::OB_gc_live))
+  auto *Statepoint = getStatepoint();
+  if (isa<UndefValue>(Statepoint))
+    return UndefValue::get(Statepoint->getType());
+
+  auto *GCInst = cast<GCStatepointInst>(Statepoint);
+  if (auto Opt = GCInst->getOperandBundle(LLVMContext::OB_gc_live))
     return *(Opt->Inputs.begin() + getDerivedPtrIndex());
-  return *(getStatepoint()->arg_begin() + getDerivedPtrIndex());
+  return *(GCInst->arg_begin() + getDerivedPtrIndex());
 }

llvm/lib/IR/Verifier.cpp

if (LandingPadInst *LandingPad =
       Check(InvokeBB->getTerminator(), "safepoint block should be well formed",
             InvokeBB);
       Check(isa<GCStatepointInst>(InvokeBB->getTerminator()),
             "gc relocate should be linked to a statepoint", InvokeBB);
     } else {
       // In all other cases relocate should be tied to the statepoint directly.
       // This covers relocates on a normal return path of invoke statepoint and
       // relocates of a call statepoint.
-      auto Token = Call.getArgOperand(0);
-      Check(isa<GCStatepointInst>(Token),
+      auto *Token = Call.getArgOperand(0);
+      Check(isa<GCStatepointInst>(Token) || isa<UndefValue>(Token),
             "gc relocate is incorrectly tied to the statepoint", Call, Token);
     }
 
     // Verify rest of the relocate arguments.
-    const CallBase &StatepointCall =
-      *cast<GCRelocateInst>(Call).getStatepoint();
+    const Value &StatepointCall = *cast<GCRelocateInst>(Call).getStatepoint();
 
     // Both the base and derived must be piped through the safepoint.
     Value *Base = Call.getArgOperand(1);
     Check(isa<ConstantInt>(Base),
           "gc.relocate operand #2 must be integer offset", Call);
 
     Value *Derived = Call.getArgOperand(2);
     Check(isa<ConstantInt>(Derived),
           "gc.relocate operand #3 must be integer offset", Call);
 
     const uint64_t BaseIndex = cast<ConstantInt>(Base)->getZExtValue();
     const uint64_t DerivedIndex = cast<ConstantInt>(Derived)->getZExtValue();
 
     // Check the bounds
-    if (auto Opt = StatepointCall.getOperandBundle(LLVMContext::OB_gc_live)) {
+    if (isa<UndefValue>(StatepointCall))
+      break;
+    if (auto Opt = cast<GCStatepointInst>(StatepointCall)
+                       .getOperandBundle(LLVMContext::OB_gc_live)) {
       Check(BaseIndex < Opt->Inputs.size(),
             "gc.relocate: statepoint base index out of bounds", Call);
       Check(DerivedIndex < Opt->Inputs.size(),
             "gc.relocate: statepoint derived index out of bounds", Call);
     }
 
     // Relocated value must be either a pointer type or vector-of-pointer type,
     // but gc_relocate does not need to return the same pointer type as the

llvm/test/Transforms/InstCombine/gc.relocate-verify.ll

+; RUN: opt -S -passes=verify < %s | FileCheck %s
+
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @check_verify_undef_token() gc "statepoint-example" {
+
+entry:
+    ; CHECK: ret i32 0
+    ret i32 0
+
+unreach:
+    ; CHECK: token undef
+    %token_call = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(token undef, i32 0, i32 0)
+    ret i32 1
+}
+
+declare i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(token, i32, i32)