Skip to content

[DebugInfo] Update policy for when to merge locations #115349

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 12, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions llvm/docs/HowToUpdateDebugInfo.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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``. For each new instruction I, its new
location should be a merge of the locations of all instructions whose output is
produced by I. 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.
Copy link
Contributor

@felipepiovezan felipepiovezan Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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
Expand All @@ -101,10 +105,15 @@ 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. Similarly, if
prior optimizations have already produced vectors ``(A1, A2)`` and
``(B2, B1)``, then we might create a ``(shufflevector (1, 0), (B2, B1))``
instruction to produce ``(B1, B2)`` for the vector ``add``; in this case we've
created two instructions to replace the original ``adds``, so both new
instructions should use the merged location.

Examples of transformations for which this rule *does not* apply include:

Expand All @@ -113,6 +122,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
Expand Down
Loading