[PATCH] D153990: [DebugInfo][RemoveDIs] Add prototype storage classes for non-instruction variable debug-info

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando accepted this revision.
Orlando added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:2100-2102
+    MDNode *UniqueArgList = uniquify();
+    if (UniqueArgList != this) {
+      replaceAllUsesWith(UniqueArgList);
----------------
jmorse wrote:
> jmorse wrote:
> > Orlando wrote:
> > > Please could you explain this change to me (why/what it's doing)? It seems the couple of months I've not touched metadata have clouded my memory.
> > I...... can't remember. Or at least, I think this comes from the work @StephenTozer did on these patches, and this deep into the metadata is out of my zone of comfort too. Any opinion on this Stephen?
> > 
> > I'll also see if we can just delete it.
> Giving this another eyeball -- previously all DIArgList's have been referred to through a single MetadataAsValue object which (AFAIUI) could be directly looked up, and whenever an operand changed that single MAV/DIArgList gets rewritten and re-uniqueified. Which is fine because with only one reference to that Metadata, it's like being RAUW'd.
> 
> However with the DebugValueUser changes in Metadata.cpp here, we're tolerating replaceable metadata references to ValueAsMetadata's and DIArgLists -- instead of funnelling connections from the Value hierarchy to the Metadata hierarchy through a VAM/MAV pair there's a web of connections. Thus, now when an operand to a DIArgList changes, we have to go through all the DPValues that refer to this DIArgList and redirect them to the new one. That means the code here changes from "this Metadata object now means something different" to "rewrite all the users to point at this new object".
> 
> Is it a good idea to use this web of connections instead of the funnel? That's something we can model/measure at a later date seeing how DPValues are going to be using it a lot.
Ok, tyvm for the explanation.


================
Comment at: llvm/lib/IR/DebugProgramInstruction.cpp:328
+  if (InsertAtHead) {
+    for (DPValue &DPV : reverse(Range)) {
+      DPValue *New = DPV.clone();
----------------
Orlando wrote:
> Orlando wrote:
> > Why reverse here? (And if it's not needed, can the `for`s be merged with a conditional insert pos?)
> ping
Ah I've got it - if `InsertAtHead` then prepend, otherwise append.


================
Comment at: llvm/unittests/IR/DebugInfoTest.cpp:777-780
+  FirstInst->DbgMarker = new DPMarker();
+  FirstInst->DbgMarker->MarkedInstr = FirstInst;
+  RetInst->DbgMarker = new DPMarker();
+  RetInst->DbgMarker->MarkedInstr = RetInst;
----------------
Orlando wrote:
> jmorse wrote:
> > Orlando wrote:
> > > Is there an API for handling this behind the scenes by adding DPValues to instructions? If not, why not, if yes, please can we test that too?
> > There is one coming in D154353, just this is the order that it made sense to stage the patches (insert shrugging emoticon here). Alright if we patch this up later?
> ping
SGTM


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

https://reviews.llvm.org/D153990