Simplify coro::salvageDebugInfo() (NFC-ish)

[aprantl]

Summary

This patch removes the hand-rolled implementation of salvageDebugInfo for cast and GEPs and replaces it with a call into llvm::salvageDebugInfoImpl().

A side-effect of this is that additional redundant convert operations are introduced, but those don't have any negative effect on the resulting DWARF expression.

[ChuanqiXu]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

2533–2536

If Op is null and we break this loop, Storage would still be no-nullptr. It means that it wouldn't trigger the condition in 2547, then there could be a potential bug to me.

[aprantl]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

2533–2536

Previously we filtered out only GEP instructions on line 2530, and so the if (!Storage) meant salvaging a GEP instruction failed. In the new code we try to salvage any instruction and so the if (!Op) mostly takes over the role of the else break. In the new code the else break only catches function arguments, constants, and other non-instructions.
There's a behavior change here in that we no longer can detect if a GEP instruction was unsalvageable. Note that we still filter for function arguments on line 2558. The new code is now rewriting DVI for a failed GEP, which the old code didn't, but that's a perfectly safe change that doesn't pessimize debug info in any way.

[ChuanqiXu]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

2533–2536

Many thanks for the detailed explanation! Now I got it. Another question is that: is the line if (!Storage) and the loop condition while(Storage) redundant now? Since it looks like that Storage wouldn't be converted to null in this loop.

[aprantl]

Simplified the loop even more based on feedback by @ChuanqiXu !

[ChuanqiXu]

LGTM.

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

while (isa<IntrinsicInst>(InsertPt))
     ++InsertPt;
   Builder.SetInsertPoint(&F->getEntryBlock(), InsertPt);
   DIExpression *Expr = DVI->getExpression();
   // Follow the pointer arithmetic all the way to the incoming
   // function argument and convert into a DIExpression.
   bool OutermostLoad = true;
   Value *Storage = DVI->getVariableLocationOp(0);
   Value *OriginalStorage = Storage;
-  while (Storage) {
-    if (auto *LdInst = dyn_cast<LoadInst>(Storage)) {
+  while (auto *Inst = dyn_cast<Instruction>(Storage)) {
+    if (auto *LdInst = dyn_cast<LoadInst>(Inst)) {
       Storage = LdInst->getOperand(0);
       // FIXME: This is a heuristic that works around the fact that
       // LLVM IR debug intrinsics cannot yet distinguish between
       // memory and value locations: Because a dbg.declare(alloca) is
       // implicitly a memory location no DW_OP_deref operation for the
       // last direct load from an alloca is necessary.  This condition
       // effectively drops the *last* DW_OP_deref in the expression.
       if (!OutermostLoad)
         Expr = DIExpression::prepend(Expr, DIExpression::DerefBefore);
       OutermostLoad = false;
-    } else if (auto *StInst = dyn_cast<StoreInst>(Storage)) {
+    } else if (auto *StInst = dyn_cast<StoreInst>(Inst)) {
       Storage = StInst->getOperand(0);
-    } else if (auto *GEPInst = dyn_cast<GetElementPtrInst>(Storage)) {
+    } else {
       SmallVector<uint64_t, 16> Ops;
       SmallVector<Value *, 0> AdditionalValues;
-      Storage = llvm::salvageDebugInfoImpl(
-          *GEPInst, Expr ? Expr->getNumLocationOperands() : 0, Ops,
+      Value *Op = llvm::salvageDebugInfoImpl(
+          *Inst, Expr ? Expr->getNumLocationOperands() : 0, Ops,
           AdditionalValues);
-      if (!Storage)
-        break;
-      // Debug declares cannot currently handle additional location
-      // operands.
-      if (!AdditionalValues.empty())
+      if (!Op || !AdditionalValues.empty()) {
+        // If salvaging failed or salvaging produced more than one location
+        // operand, give up.
         break;
+      }
+      Storage = Op;
       Expr = DIExpression::appendOpsToArg(Expr, Ops, 0, /*StackValue*/false);
-    } else if (auto *BCInst = dyn_cast<llvm::BitCastInst>(Storage))
-      Storage = BCInst->getOperand(0);
-    else
-      break;
     }
-  if (!Storage)
-    return;
+  }
         
   // Store a pointer to the coroutine frame object in an alloca so it
   // is available throughout the function when producing unoptimized
   // code. Extending the lifetime this way is correct because the
   // variable has been declared by a dbg.declare intrinsic.
   //
   // Avoid to create the alloca would be eliminated by optimization
   // passes and the corresponding dbg.declares would be invalid.
   if (!ReuseFrameSlot && !EnableReuseStorageInFrame)

llvm/test/Transforms/Coroutines/coro-debug.ll

 ; CHECK: define internal fastcc void @f.resume(%f.Frame* noalias nonnull align 8 dereferenceable(32) %FramePtr) #0 !dbg ![[RESUME:[0-9]+]]
 ; CHECK: entry.resume:
 ; CHECK: %[[DBG_PTR:.*]] = alloca %f.Frame*
 ; CHECK: call void @llvm.dbg.declare(metadata %f.Frame** %[[DBG_PTR]], metadata ![[RESUME_COROHDL:[0-9]+]], metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst,
 ; CHECK: call void @llvm.dbg.declare(metadata %f.Frame** %[[DBG_PTR]], metadata ![[RESUME_X:[0-9]+]], metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, [[EXPR_TAIL:.*]])
 ; CHECK: call void @llvm.dbg.declare(metadata %f.Frame** %[[DBG_PTR]], metadata ![[RESUME_DIRECT:[0-9]+]], metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, [[EXPR_TAIL]])
 ; CHECK: store %f.Frame* {{.*}}, %f.Frame** %[[DBG_PTR]]
 ; CHECK-NOT: alloca %struct.test*
-; CHECK: call void @llvm.dbg.declare(metadata i32 0, metadata ![[RESUME_CONST:[0-9]+]], metadata !DIExpression())
+; CHECK: call void @llvm.dbg.declare(metadata i8 0, metadata ![[RESUME_CONST:[0-9]+]], metadata !DIExpression(DW_OP_LLVM_convert, 8, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed))
 ; Note that keeping the undef value here could be acceptable, too.
 ; CHECK-NOT: call void @llvm.dbg.declare(metadata i32* undef, metadata !{{[0-9]+}}, metadata !DIExpression())
 ; CHECK: call void @coro.devirt.trigger(i8* null)
 ; CHECK: define internal fastcc void @f.destroy(%f.Frame* noalias nonnull align 8 dereferenceable(32) %FramePtr) #0 !dbg ![[DESTROY:[0-9]+]]
 ; CHECK: define internal fastcc void @f.cleanup(%f.Frame* noalias nonnull align 8 dereferenceable(32) %FramePtr) #0 !dbg ![[CLEANUP:[0-9]+]]
 
 ; CHECK: ![[ORIG]] = distinct !DISubprogram(name: "f", linkageName: "flink"