[PATCH] D98644: [DebugInfo] Fix incorrect handling of variadic debug values

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer created this revision.
StephenTozer added reviewers: probinson, aprantl, djtodoro, vsk, dblaikie, dstenb.
StephenTozer added a project: debug-info.
Herald added subscribers: dexonsmith, hiraditya.
StephenTozer requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

This patch fixes two issues observed upon applying D91722 <https://reviews.llvm.org/D91722> to current main:

Firstly, `SelectionDAG::transferDebugValues` incorrectly updates the `SDDbgValue`'s dependencies list. Specifically, this class maintains two closely related lists; the list of `SDDbgOperands`(which, for nodes, consist of an `SDNode` //and// a result number, `(Node, ResNo)`), and the list of `SDNode`s that it depends on (this list contains duplicates, which is important here). The latter list is derived from the former (this isn't true for all `SDDbgValues`, but it is true for all variadic cases), so we can expect each node operand to have a corresponding dependency. When we transfer debug values, currently we replace each operand that matches the `(Node, ResNo)` with the target node and result number, but we then replace every dependency of `Node` with the target node as well. This is incorrect if we have operands `[(Node1, 0), (Node1, 1)]`, as the dependencies will be `[Node1, Node1]`, so when we want to replace `(Node1, 0)` with `(Node2, 0)`, we end up with operands [(Node2, 0), (Node1, 1)]` (correct), and dependencies `[Node2, Node2]` (incorrect).

My current approach to fixing this is to change the dependency handling in `SDDbgValue` so that we derive the dependencies from the operands where possible. This is done by keeping a list of `AdditionalDependencies`, which contains all of the debug value's dependencies that aren't used directly as operands. When we fetch the list of dependencies, we fetch a combination of the nodes used by location operands and the additional dependencies; this means that we can freely update the debug operands without worrying about keeping the dependencies array in sync. Technically this approach could be taken further, as all the additional dependencies actually come from frame index location operands, but this kind of tracking isn't handled even for the existing cases; as this is currently holding up the main variadic patch, it seemed prudent to stick to the simple case for now.

The second issue is that LoopStrengthReduce, when trying to "repair" undef debug values by replacing them with equal values, must first restore the original location metadata. Without variadic debug values, no active restore was necessary, because you would always have a single undef location operand which would be replaced; because of the updated salvaging, it is now possible for the number of operands to change. This causes errors because we cannot identify which post-salvage operands map to the original operands. Restoring the original location metadata before attempting to repair it removes this issue.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D98644

Files:
  llvm/include/llvm/IR/IntrinsicInst.h
  llvm/lib/CodeGen/SelectionDAG/SDNodeDbgValue.h
  llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D98644.330723.patch
Type: text/x-patch
Size: 10080 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210315/96cd6fe4/attachment.bin>