[Debug] [Coroutines] Add deref operator for non complex expression

[ChuanqiXu]

Summary

Background:

When we construct coroutine frame, we would insert a dbg.declare intrinsic for it:

%hdl = call void @llvm.coro.begin() ; would return coroutine handle
call void @llvm.dbg.declare(metadata ptr %hdl, metadata ![[DEBUG_VARIABLE: __coro_frame]], metadata !DIExpression())

And in the splitted coroutine, it looks like:

define void @coro_func.resume(ptr *hdl) {
entry.resume:
    call void @llvm.dbg.declare(metadata ptr %hdl, metadata ![[DEBUG_VARIABLE: __coro_frame]], metadata !DIExpression())
}

And we would salvage the debug info by inserting a new alloca here:

define void @coro_func.resume(ptr %hdl) {
entry.resume:
    %frame.debug = alloca ptr
    call void @llvm.dbg.declare(metadata ptr %frame.debug, metadata ![[DEBUG_VARIABLE: __coro_frame]], metadata !DIExpression())
    store ptr %hdl, %frame.debug
}

But now, the problem comes since the dbg.declare refers to the address of that alloca instead of actual coroutine handle. I saw there are codes to solve the problem but it only applies to complex expression only. I feel if it is OK to relax the condition to make it work for __coro_frame.

[jmorse]

This seems fine to me -- something very similar happens with NRVO in C++, a dbg.declare(..., DW_OP_deref) is created for the implicit output argument. There's a test demonstrating this in clang/test/CodeGenCXX/debug-info-nrvo.cpp.

The whole interplay between levels of DW_OP_deref and whether there are already opcodes in the expression is pretty miserable; what scenarios is salvageDebugInfo called in? I'm trying to reason about when the isComplex test might be necessary. Also, IMO, you should be able to assert that Expr is nonzero. AFAIUI dbg.declare's need to have the expression argument non-null, so any null Expr found here is a problem with the input IR.

[ChuanqiXu]

Address comments.

[ChuanqiXu]

In D126277#3540094, @jmorse wrote:

what scenarios is salvageDebugInfo called in? I'm trying to reason about when the isComplex test might be necessary.

Background: compiler would construct new structs called coroutine frames for the coroutines. It is necessary since a coroutine would be split into multiple functions in the middle end so we need a state to record the information. So the following one:

coro f(int a) {
   a++;
   co_await something();
   a++;
   co_return a;
}

would be translated to:

coro f(int a) {
    frame_ty *frame = malloc(size);
    store `a` to someplace of frame
    %a.tmp = load `a` from frame
    %a.tmp += 1
    store %a.tmp to frame
    ...
}

then it would be split into:

coro f(int a) {
    frame_ty *frame = malloc(size);
    store `a` to someplace of frame
    return frame
}

void f.resume(frame_ty* frame) {
    %a.tmp = load `a` from frame
    %a.tmp += 1
    store %a.tmp to frame

unreachable: // which would be removed finally
    frame_ty *frame = malloc(size);
    store `a` to someplace of frame
    ...
}

then if we add dbg intrinsics, it would look like:

coro f(int a) {
    frame_ty *frame = malloc(size);
    call @dbg.declare(frame, "frame_var", DIExpression())
    store `a` to someplace of frame
    call @dbg.declare(the address of `a` in frame, "a", DIExpression())
    return frame
}

void f.resume(frame_ty* frame) {
    %a.tmp = load `a` from frame
    %a.tmp += 1
    store %a.tmp to frame

unreachable:
    frame_ty *d_frame = malloc(size);
    call @dbg.declare(d_frame, "frame_var", DIExpression())
    store `a` to someplace of d_frame
    call @dbg.declare(the address of `a` in d_frame, "a", DIExpression())
    ...
}

Then we call salvageDebugInfo here to salvage the debug information from unreachable bb to the entry of the f.resume function. Then the main work it tries to do is to replace d_frame to the argument. And I remember the reason why we would create an additional alloca is that the argument couldn't be the address argument of @dbg.declares

[ChuanqiXu]

@aprantl @dblaikie @jmorse gentle ping~

[jmorse]

(All away for the UK holiday week sorry),

Thanks for the detail on how salvaging happens with coroutines -- I was curious why it would depend on what was already in the DIExpression. The deref is indeed necessary because the pointer is in an alloca. I suspect the check for isComplex() wasn't completely necessary (but only because of DIExpression quirks, unfortunately).

LGTM, but could you put an extra CHECK in where I've marked to capture FramePtr_RESUME using the alloca instruction. When reading the test, it wasn't clear to me where the dbg.declare was going to point, this will make it easier for future readers to understand.

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

12

Could you put a CHECK in here for the FramePtr_RESUME variable that checks it's coming from an alloca?

[ChuanqiXu]

In D126277#3562899, @jmorse wrote:

(All away for the UK holiday week sorry),

Thanks for the detail on how salvaging happens with coroutines -- I was curious why it would depend on what was already in the DIExpression. The deref is indeed necessary because the pointer is in an alloca. I suspect the check for isComplex() wasn't completely necessary (but only because of DIExpression quirks, unfortunately).

LGTM, but could you put an extra CHECK in where I've marked to capture FramePtr_RESUME using the alloca instruction. When reading the test, it wasn't clear to me where the dbg.declare was going to point, this will make it easier for future readers to understand.

Thanks for reviewing!

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 SkipOutermostLoad = !isa<DbgValueInst>(DVI);
   Value *Storage = DVI->getVariableLocationOp(0);
   Value *OriginalStorage = Storage;
+
   while (auto *Inst = dyn_cast_or_null<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
if (auto *Arg = dyn_cast<llvm::Argument>(Storage)) {
       Storage = Cached;
       // FIXME: LLVM lacks nuanced semantics to differentiate between
       // memory and direct locations at the IR level. The backend will
       // turn a dbg.declare(alloca, ..., DIExpression()) into a memory
       // location. Thus, if there are deref and offset operations in the
       // expression, we need to add a DW_OP_deref at the *start* of the
       // expression to first load the contents of the alloca before
       // adjusting it with the expression.
-      if (Expr && Expr->isComplex())
-        Expr = DIExpression::prepend(Expr, DIExpression::DerefBefore);
     }
 
   DVI->replaceVariableLocationOp(OriginalStorage, Storage);
   DVI->setExpression(Expr);
   // We only hoist dbg.declare today since it doesn't make sense to hoist
   // dbg.value or dbg.addr since they do not have the same function wide
   // guarantees that dbg.declare does.
   if (!isa<DbgValueInst>(DVI) && !isa<DbgAddrIntrinsic>(DVI)) {

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

 ; RUN: opt < %s -passes='module(coro-early),cgscc(coro-split,coro-split)' -S | FileCheck %s
 
 ; Checks whether the dbg.declare for `__coro_frame` are created.
 
 ; CHECK-LABEL: define void @f(
 ; CHECK:       coro.init:
 ; CHECK:        %[[begin:.*]] = call noalias nonnull i8* @llvm.coro.begin(
 ; CHECK:        call void @llvm.dbg.declare(metadata i8* %[[begin]], metadata ![[CORO_FRAME:[0-9]+]], metadata !DIExpression())
 ; CHECK:        %[[FramePtr:.*]] = bitcast i8* %[[begin]] to
 ;
 ; CHECK:       define internal fastcc void @f.resume(
 ; CHECK:       entry.resume:
-; CHECK:            call void @llvm.dbg.declare(metadata %f.Frame** %[[FramePtr_RESUME:.*]], metadata ![[CORO_FRAME_IN_RESUME:[0-9]+]], metadata !DIExpression()
+; CHECK:            %[[FramePtr_RESUME:.*]] = alloca %f.Frame*
+; CHECK:            call void @llvm.dbg.declare(metadata %f.Frame** %[[FramePtr_RESUME]], metadata ![[CORO_FRAME_IN_RESUME:[0-9]+]], metadata !DIExpression(DW_OP_deref)
 ;
 ; CHECK-DAG: ![[FILE:[0-9]+]] = !DIFile(filename: "coro-debug.cpp"
 ; CHECK-DAG: ![[RAMP:[0-9]+]] = distinct !DISubprogram(name: "foo", linkageName: "_Z3foov",
 ; CHECK-DAG: ![[RAMP_SCOPE:[0-9]+]] = distinct !DILexicalBlock(scope: ![[RAMP]], file: ![[FILE]], line: 23
 ; CHECK-DAG: ![[CORO_FRAME]] = !DILocalVariable(name: "__coro_frame", scope: ![[RAMP_SCOPE]], file: ![[FILE]], line: [[PROMISE_VAR_LINE:[0-9]+]], type: ![[FRAME_TYPE:[0-9]+]], flags: DIFlagArtificial)
 ; CHECK-DAG: ![[FRAME_TYPE]] = !DICompositeType(tag: DW_TAG_structure_type, name: "__coro_frame_ty", scope: ![[RAMP]], {{.*}}elements: ![[ELEMENTS:[0-9]+]]
 ; CHECK-DAG: ![[ELEMENTS]] = !{![[RESUME_FN:[0-9]+]], ![[DESTROY_FN:[0-9]+]], ![[PROMISE:[0-9]+]], ![[INT64_0:[0-9]+]], ![[DOUBLE_1:[0-9]+]], ![[INT32_2:[0-9]+]], ![[INT32_3:[0-9]+]], ![[STRUCT_4:[0-9]+]], ![[CORO_INDEX:[0-9]+]]
 ; CHECK-DAG: ![[RESUME_FN]] = !DIDerivedType(tag: DW_TAG_member, name: "__resume_fn"{{.*}}, flags: DIFlagArtificial