[PATCH] D90772: [Coroutines] Add missing llvm.dbg.declare's to cover more allocas

[Bruno Cardoso Lopes via Phabricator via llvm-commits]

bruno added a comment.

Hi Jeremy, thanks for taking a deep look here.

> Hrrrmmmm. Just to confirm my understanding, in tests like the one modified in this patch, all those allocas (%i, %j, %x) are transformed into GEPs from FramePtr, which is malloc'd / new'd or otherwise not on the stack?

That's right.

> Normally dbg.declare is supposed to refer to a static alloca -- that way we can easily identify stack variables and not bother tracking them through optimisations. Sometimes this isn't true though (certain ABIs pass in memory for return values apparently), in which case the dbg.declare gets silently transformed into something like `dbg.value(%0, [...], DW_OP_deref)`, which is what seems to happen here.

Can you clarify what you mean by "silently transformed into something like `dbg.value...`"? In what stage do you see that happening? Those don't show up for me unless the local variables are already promoted to regs.

> That should be fine: however as you've already spotted, the debug intrinsic needs to dominate the instructions that receive the variable location, and that doesn't seem to be the case in the given example. Here's a gist <https://gist.github.com/jmorse/4850a987bfc8d4fed50c622a0b8e7c46> with the input test.cpp at the bottom, and IR for foo.resume on IR line 693 onwards. I've compiled this with a ~6 week old clang master and `-emit-llvm -S -g -O0 -fcoroutines-ts -stdlib=libc++`. I'm not familiar with the coroutine implementation details and assumed:
>
> - foo.resume is the function we care about as it's the first function gdb stops in when I set breakpoints in the "foo" function,
> - The await.ready block is the main part of the foo.ready function, and everything else is coroutine plumbing.

Yep.

> From the entry block, there's a path through the blocks thus:
>
>   resume.entry
>   resume.1
>   resume.1.landing
>   AfterCoroSuspend50
>   await.ready
>
> Which reaches the await.ready block without going through any debug intrinsics. The variable locations won't be propagated into the await.ready block because of this path, as there's no information about variable locations on it.  This matches what dwarfdump reports: there's a variable location for _some_ of the function, but not all of it, and not the part that the developer wants to step through.

If you look at the final IR, this is true and it actually confirms why it wasn't working before, thanks! However, at the point where `insertSpills` is called and the logic on this patch runs, there's no resume function just yet, since it's operating on the original `foo`, and in that context `init.ready` (which already has a `dbg.declare`s) still dominates `await.ready`.

Given that, another approach would be to redo this logic on top of the already formed resume function and only re-insert the `dbg.declare`s in BBs that are not dominated. One possible drawback of this approach is that it would require applying the same logic to init, resume and destroy and going for another CFG walk for each alloca spill. Doesn't sound super expensive but the convenience on doing it in the original function instead sounds compelling. What's the impact of having reductant `dbg.declare`s in already dominated paths? Is there a pass that cleans those up? Thoughts?

> I'm not familiar with the coroutine implementation details, glad to be corrected. (CC @Orlando @chrisjackson , this is an example of where redesigned intrinsics would definitely need to refer to variables in an arbitrary memory location, ouch).




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

https://reviews.llvm.org/D90772