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

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

fdeazeve added a comment.

While debugging a crash in the compiler (see my comment about GlobalVariable) below, I found this patch and I am struggling to understand if we are doing the right thing here.

Besides the crash, I am not sure what this patch is correct with respect to the debug intrinsics. Consider this:

  %indirect_storage = ptr ...
  %AliveAccrossAplit = load ptr %indirect_storage
  call void llvm.dbg.declare(%indirect_storage, ...)
  
  ... split point ...
  
  use %UseAliveAccrossAplit

With this patch, we rewrite the above into:

  %indirect_storage = ptr ...
  %AliveAccrossSplit = load ptr %indirect_storage
  call void llvm.dbg.declare(%indirect_storage, !MyVar, ...)
  
  ... split point ...
  
  %reload = ... reload of %AliveAccrossSplit inside the coroutine frame ...
  call void llvm.dbg.declare(%reload, !MyVar, ...)

Note that the dbg.declare after the rewrite is *not* equivalent to the original intrinsic: we are now claiming the address of !MyVar is `%reload`, and `%reload == %AliveAccrossSplit`.

Does this make sense? In fact, the debug instruction that is being created doesn't even show up in the test added by this patch. If we run this test on a debugger and put a breakpoint in this line:

  -> 1881           coro::salvageDebugInfo(ArgToAllocaMap, DDI, Shape.OptimizeFrame,
     1882                                  false /*UseEntryValue*/);

We should this:

  (lldb) p CurrentBlock->dump()
  
  resume:                                           ; preds = %AfterCoroSuspend
    %this1.reload.addr = getelementptr inbounds %foo.Frame, ptr %hdl, i32 0, i32 4
    %this1.reload = load ptr, ptr %this1.reload.addr, align 8
    call void @llvm.dbg.declare(metadata ptr %this1.reload, metadata !34, metadata !DIExpression()), !dbg !36
    call void @bar(ptr %this1)
    br label %cleanup

Note that this dbg.declare doesn't survive the end of this test (it is not in the final output).

I'm fairly sure that if we revert this patch and rerun the test, it will still pass.



================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1852
+        // be a direct dbg.declare. Walk up the load chain to find one from an
+        // alias.
+        if (F->getSubprogram()) {
----------------
I'm not sure alias is the right term here: if we walk up a load chain, we are not finding aliases, instead we're adding levels of pointer indirections. These pointers don't alias, they point to different addresses.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1858
+            // Only consider ptr to ptr same type load.
+            if (LdInst->getPointerOperandType() != LdInst->getType())
+              break;
----------------
What is this load trying to check? `LdInst->getPointerOperandType()` always returns the type `ptr`.
This is the same as `if(!isa<PointerType<(LdInst->getType())`, was this the intention?

I was a bit confused with the wording of the comment about, would appreciate your help rewording it.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1861
+            CurDef = LdInst->getPointerOperand();
+            DIs = FindDbgDeclareUses(CurDef);
+          }
----------------
Note that this will crash the compiler if `CurDef` is a `GlobalVariable`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146543