[PATCH] D146543: [Coroutines] Look for dbg.declare for temp spills

[Felipe de Azevedo Piovezan via Phabricator via llvm-commits]

fdeazeve added a comment.

Thank you for the quick response!

>> Note that this will crash the compiler if CurDef is a GlobalVariable.
>
> You are right, and we also observed the crash and fixed in https://reviews.llvm.org/D157423. Since the original change is targeting a very specific case, the fix puts more restriction on the case it can affect.

Thank you for the pointers, glad to hear it was fixed in a subsequent commit. I was working on a slightly older version of the codebase.

> Do you have any suggestions on how can we make it more generic and applicable to more use cases?



> After:
>
>   define internal fastcc void @foo.resume(ptr noundef nonnull align 8 dereferenceable(32) %hdl) !dbg !38 {
>   entry.resume:
>     %hdl.debug = alloca ptr, align 8
>     call void @llvm.dbg.declare(metadata ptr %hdl.debug, metadata !41, metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 24)), !dbg !42
>     call void @llvm.dbg.declare(metadata ptr %hdl.debug, metadata !40, metadata !DIExpression(DW_OP_deref)), !dbg !42
>   
>   ... ...
>   
>   !40 = !DILocalVariable(name: "__coro_frame", scope: !38, file: !1, type: !22, flags: DIFlagArtificial)
>   !41 = !DILocalVariable(name: "this", arg: 1, scope: !38, type: !35, flags: DIFlagArtificial | DIFlagObjectPointer)

I understand the problem the patch is solving, but I haven't been able to figure out how we are going from that dbg.declare that I mentioned to the one that ends up in the final output of the compiler. Do you see the problem that I was trying to point to? The _first_ dbg.declare that we create -- the one shown in the output of `p CurrentBlock->dump()` is not equivalent to the original dbg.declare. And yet, somehow, this "invalid" dbg.declare disappears and the correct one shows up later. To be very specific, when we do:

  DIBuilder(*CurrentBlock->getParent()->getParent(), AllowUnresolved)
                .insertDeclare(CurrentReload, DDI->getVariable(),
                               DDI->getExpression(), DDI->getDebugLoc(),
                               &*Builder.GetInsertPoint());

The `CurrentReload` seems incorrect if we stripped some of the loads along the way. Does that make sense?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146543/new/

https://reviews.llvm.org/D146543