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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse marked an inline comment as done.
jmorse added a comment.

(Missed inline comments being submitted...)



================
Comment at: llvm/include/llvm/IR/DebugProgramInstruction.h:112-114
+  DILocalVariable *Variable;
+  DIExpression *Expression;
+  DebugLoc DbgLoc;
----------------
Orlando wrote:
> Looks like these fields have 'getters' already - should they be private? Or, if they don't change after ctor we could make them const instead.
Shifted them up to the "private" area of the class


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:2100-2102
+    MDNode *UniqueArgList = uniquify();
+    if (UniqueArgList != this) {
+      replaceAllUsesWith(UniqueArgList);
----------------
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.


================
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:
> 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?


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

https://reviews.llvm.org/D153990