[PATCH] D154353: [DebugInfo][RemoveDIs] Implement behind-the-scenes debug-info maintenance in splice / moveBefore / insertBefore APIs

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

Orlando added inline comments.


================
Comment at: llvm/lib/IR/Instruction.cpp:118-119
+
+  // No need to manually update DPValues: if we insert after an instruction
+  // position, then we can never have any DPValues on "this".
+  if (DestParent->IsNewDbgInfoFormat)
----------------
jmorse wrote:
> Orlando wrote:
> > jmorse wrote:
> > > Orlando wrote:
> > > > I'm a bit confused about what this comment is saying, please can you explain further (here)?
> > > My recollection is that there are no scenarios where DPValues can change what instruction they're attached to as a result of insertAfter, for example:
> > > 
> > >     call dbg.value
> > >     %foo = add
> > >     call dbg.value
> > >     %bar = sub
> > >     call dbg.value
> > > 
> > > Inserting after %foo or after %bar doesn't cause an instruction be inserted inbetween dbg.values and the next instruction. All the dbg.values continue to "happen" at the next instruction, and that's preserved using DPValues too.
> > > 
> > > Of course, if the insert position of an instruction is a dbg.value intrinsic then this is no longer true. Not allowing people to do that is one of the design goals though: doing that is always a bug.
> > That makes sense, though I still find the code comment confusing. If there's anything you can do to make it clearer that might help. YMMV
> Ah yeah, I was going to update it, whoops. How about instead:
> 
> "There cannot be any debug-info records in the gap between this instruction, and the one we're inserting right now, therefore no DPValue maintenance is required".
> 
> ?
IMO "There cannot be any debug-info records in the gap between this instruction and the one we're inserting" feels slightly misleading as there might be debug-info records, it's just that everything still ends up in the correct order without intervention IIUC.

How about something like:
"Inserting 'this' after 'InsertPos', 'this' comes before any debug-info records attached to the next instruction, so no debug-info updates are necessary."

I'm not 100% happy with that either. I feel I'm splitting hairs here though, so if you're not a fan of this iteration I'm happy for you to go with your suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154353