[PATCH] D80052: [docs] Sketch outline for HowToUpdateDebugInfo.rst

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added inline comments.


================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:27
+
+When an ``Instruction`` is deleted, its debug uses change to ``undef``.  This
+is a loss of debug info: the value of a one or more source variables becomes
----------------
aprantl wrote:
> vsk wrote:
> > TWeaver wrote:
> > > Hi and thanks for this!
> > > 
> > > I'm not sure this line is actually correct.
> > > 
> > > It's my assumption that when a value pointed at by an intrinsic is deleted, the intrinsic is left with a valueless ValueAsMetaData() operand.
> > > 
> > > This empty ValueAsMetaData points at an empty metadata node, thus leaving us with an 'empty' intrinsic.
> > > 
> > > These empty intrinsics are usually removed from the program by later passes that clean up or remove 'junk' from the instruction stream.
> > > 
> > > My reading of this line gives me the impression that if I delete a value pointed to by an intrinsic, the intrinsic is auto-magically fixed up to be undef.
> > > 
> > > This may not have been your intent but it's how it reads to me.
> > > 
> > > I've fixed a bug in the Reassociate pass related to this issue in:
> > > 
> > > https://reviews.llvm.org/rG41c3f76dcd0daeec60eddfcb56008a31ad6e8738
> > Thanks for pointing this out. This seems bad.
> > 
> > A valueless dbg.value is treated as trivially dead (see `wouldInstructionBeTriviallyDead`). So unless specific care is taken to call `salvageDebugInfoOrMarkUndef`, deleting an Instruction -- and then running any instruction simplification helper -- results in inaccurate debug info.
> > 
> > @aprantl @dexonsmith can we change Instruction deletion s.t. any ValueAsMetadata nodes pointing to the dying value change to undef?
> Short answer: I don't know yet, we'll have to figure this out.
> @aprantl @dexonsmith can we change Instruction deletion s.t. any ValueAsMetadata nodes pointing to the dying value change to undef?
> 

Sounds like a great idea to me (although I haven't looked at the IR implications of that recently).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80052