[DebugInfo] Update policy for when to merge locations

[SLTozer]

Following discussions on PR #114231 this patch changes the policy on merging locations, making the rule that new instructions should use a merge of the locations of all the instructions whose output is produced by the new instructions; in the case where only one instruction's output is produced, as in most InstCombine optimizations, we use only that instruction's location.

Happy to take suggestions on the wording from this one - I've not found an up-front explanation that makes it totally clear, but the examples hopefully help to clarify.

[llvmbot]

@llvm/pr-subscribers-debuginfo

Author: Stephen Tozer (SLTozer)

Changes

Following discussions on PR #114231 this patch changes the policy on merging locations, making the rule that new instructions should use a merge of the locations of all the instructions whose output is produced by the new instructions; in the case where only one instruction's output is produced, as in most InstCombine optimizations, we use only that instruction's location.

Happy to take suggestions on the wording from this one - I've not found an up-front explanation that makes it totally clear, but the examples hopefully help to clarify.

---

Full diff: https://github.com/llvm/llvm-project/pull/115349.diff

1 Files Affected:

• (modified) llvm/docs/HowToUpdateDebugInfo.rst (+16-7)

diff --git a/llvm/docs/HowToUpdateDebugInfo.rst b/llvm/docs/HowToUpdateDebugInfo.rst
index f7db92d58f4356..2d2323634b4f07 100644
--- a/llvm/docs/HowToUpdateDebugInfo.rst
+++ b/llvm/docs/HowToUpdateDebugInfo.rst
@@ -76,9 +76,13 @@ When to merge instruction locations
 -----------------------------------
 
 A transformation should merge instruction locations if it replaces multiple
-instructions with a single merged instruction, *and* that merged instruction
-does not correspond to any of the original instructions' locations. The API to
-use is ``Instruction::applyMergedLocation``.
+instructions with one or more new instructions, *and* the new instruction(s)
+produce the output of more than one of the original instructions. The API to
+use is ``Instruction::applyMergedLocation``, and the new location should be a
+merge of the locations of all the instructions whose output is produced in the
+new instructions; typically, this includes any instruction being RAUWed by a new
+instruction, and excludes any instruction that only produces an intermediate
+value used by the RAUWed instruction.
 
 The purpose of this rule is to ensure that a) the single merged instruction
 has a location with an accurate scope attached, and b) to prevent misleading
@@ -101,10 +105,10 @@ Examples of transformations that should follow this rule include:
 * Merging identical loop-invariant stores (see the LICM utility
   ``llvm::promoteLoopAccessesToScalars``).
 
-* Peephole optimizations which combine multiple instructions together, like
-  ``(add (mul A B) C) => llvm.fma.f32(A, B, C)``.  Note that the location of
-  the ``fma`` does not exactly correspond to the locations of either the
-  ``mul`` or the ``add`` instructions.
+* Scalar instructions being combined into a vector instruction, like
+  ``(add A1, B1), (add A2, B2) => (add (A1, A2), (B1, B2))``. As the new vector
+  ``add`` computes the result of both original ``add`` instructions
+  simultaneously, it should use a merge of the two locations.
 
 Examples of transformations for which this rule *does not* apply include:
 
@@ -113,6 +117,11 @@ Examples of transformations for which this rule *does not* apply include:
   ``zext`` is modified but remains in its block, so the rule for
   :ref:`preserving locations<WhenToPreserveLocation>` should apply.
 
+* Peephole optimizations which combine multiple instructions together, like
+  ``(add (mul A B) C) => llvm.fma.f32(A, B, C)``. Note that the result of the
+  ``mul`` no longer appears in the program, while the result of the ``add`` is
+  now produced by the ``fma``, so the ``add``'s location should be used.
+
 * Converting an if-then-else CFG diamond into a ``select``. Preserving the
   debug locations of speculated instructions can make it seem like a condition
   is true when it's not (or vice versa), which leads to a confusing

[dwblaikie]

SGTM, but please wait for one or two other approvals/reviews

[felipepiovezan]

with one or more new instructions

The new text is covering "new instructions" (plural!) but AFAICT we don't have an example of that in the text, which would help clarify something confusing in the text:

The API to use is Instruction::applyMergedLocation
and the new location should be a merge of the locations
of all the instructions whose output is produced in the
new instructions

We talk about the location, but if we are in the plural case there would be possibly different merged locations, right?

I think a more precise wording is:

The API to use is Instruction::applyMergedLocation. For each new instruction I, its new location should be a merge of the locations of all instructions whose output is produced by I.

[jmorse]

Sounds like a good plan to me, wording seems fine when the existing feedback is included.

[jryans]

Looks good to me overall, so should be ready to go once other comments have been resolved. Thanks for working on these details.

[SLTozer]

I've added an example for multiple merges - it's not something that actually happens in the compiler right now, primarily because DILocations aren't handled correctly through a lot of vectorizer operations (vector adds seem to just take a location from one of the input adds, for example). There is an actual example in AMDGPULibCalls::fold_sincos, where we create several instructions to replace separate sin and cos instructions with a merged sincos call, but the logic is a little obtuse for a small example imo.