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

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

Orlando added a comment.

New changes LGTM. There are still a few outstanding questions to look at (including the inline question for @StephenTozer) so I'll hold off on Accepting just yet.



================
Comment at: llvm/include/llvm/IR/BasicBlock.h:65
+  /// block returns to a cannonical form when the terminator is re-inserted.
+  DPMarker TrailingDPValues;
 
----------------
jmorse wrote:
> Orlando wrote:
> > How frequently do we expect TrailingDPValues to be used? Is it worth having this heap allocated instead to save some bits?
> Yuh; in fact, given that this is a transitory state that crops up rarely, we can probably put a map in Function rather than making every block pay a penalty.
SGTM, IMO it's ok to put that change in another patch if it makes things easier, since this does already all work and has been performance tested etc.


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


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


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

https://reviews.llvm.org/D153990