[PATCH] D58453: [DebugInfo][CGP] Limit placeDbgValues movement of dbg.value intrinsics

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

Hmmm.  It appears that you are detaching the notion of here-the-value-is-computed from here-the-value-becomes-associated-with-this-variable. Usually these are concurrent in an "assignment" to the variable.
Separating these concerns means that if a value computation is hoisted, it might not be appropriate to hoist the association with the variable.  That is, the "assignment" effect maybe should stay where it was, even if there's no specific instruction left in that position that implements that assignment.
I could see this situation arising in a variety of circumstances. CSE, various sorts of value-propagation (constant or not), passing a value to an inlined function parameter, yada yada.

So, this patch says, as long as the association (dbg.value) is dominated by the computation, assume the association is in the right place and don't move it.  Dominance is certainly the correct prerequisite for dbg.value, so in that respect the patch seems reasonable, given the separation of concerns I mentioned previously. But...

I suspect there are a bunch of places in LLVM that encode knowledge of the "value computation is assignment" paradigm, which you are apparently trying to get rid of, and I suppose the other patches you have up for review are all along this same line.  Rooting out all those places will take some doing.  I am also somewhat concerned that you can leave a dbg.value lying around with no direct relationship to the rest of the block it's in, which makes the effect of other optimizations potentially vague and ill-defined.
Was there any llvm-dev discussion about this paradigm shift?  I have been very distracted the past couple of months so it might have happened without my noticing or really paying attention to it.  While I think it is probably a net positive in the evolution of debug info for optimized code, I do want to make sure the Usual Suspects are all attuned to it and supportive.  It would also be well worthwhile putting together some words for the description of dbg.value in LangRef about this (which does not have to be part of this patch).


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

https://reviews.llvm.org/D58453