[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,

> However if the dbg.declare can't be tracked back to a stack slot, it becomes a "DBG_VALUE" machine instruction (equivalent to a dbg.value intrinsic):
>
>   DBG_VALUE %26, 0, !704, !DIExpression(), debug-location !706  
>
> The former doesn't need to worry about control flow because the variable is always homed in a stack slot; the latter does need to worry about control flow. I imagine that everything to do with coroutines will take the latter path.

I see now, thanks for sharing!

>> What's the impact of having reductant dbg.declares in already dominated paths? Is there a pass that cleans those up? Thoughts?
>
> Zero functional change, and  a tiny performance cost from having one extra metadata instruction hanging around. If you go down this route, I'd recommend using the `dbg.value` intrinsic with a DIExpression with a single DW_OP_deref -- this is effectively what `dbg.declare` becomes as shown above, and avoids any unexpected surprises if it turns out some code somewhere really does expect only one `dbg.declare` to exist.

Gotcha, I like your recommendation, let's prevent unexpected surprises. I've tested the approach and works the same in the final debugging experience. Will update the patch.


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

https://reviews.llvm.org/D90772