Refine the examples in the debug info document

[Apochens]

This PR modifies the examples of section "When to merge instruction locations" in HowToUpdateDebugInfo according to the discussion, removing one misleading counterexample and refining the description of hoisting identical instructions.

[llvmbot]

@llvm/pr-subscribers-debuginfo

Author: Shan Huang (Apochens)

Changes

This PR modifies the examples of section "When to merge instruction locations" in HowToUpdateDebugInfo according to the discussion, removing one misleading counterexample and refining the description of hoisting identical instructions.

---

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

1 Files Affected:

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

diff --git a/llvm/docs/HowToUpdateDebugInfo.rst b/llvm/docs/HowToUpdateDebugInfo.rst
index c64b5d1d0d98b6..1e36c0c45c8490 100644
--- a/llvm/docs/HowToUpdateDebugInfo.rst
+++ b/llvm/docs/HowToUpdateDebugInfo.rst
@@ -91,8 +91,11 @@ misattributed to a block containing one of the instructions-to-be-merged.
 
 Examples of transformations that should follow this rule include:
 
-* Merging identical loads/stores which occur on both sides of a CFG diamond
-  (see the ``MergedLoadStoreMotion`` pass).
+* Hoisting identical instructions from successors of a conditional branch. For
+  example, merging identical loads/stores which occur on both sides of a CFG
+  diamond (see the ``MergedLoadStoreMotion`` pass). If there are more than one
+  group of identical instructions hoisted, apply merging instruction locations
+  for each single merged instruction.
 
 * Merging identical loop-invariant stores (see the LICM utility
   ``llvm::promoteLoopAccessesToScalars``).
@@ -115,11 +118,6 @@ Examples of transformations for which this rule *does not* apply include:
   single-stepping experience. The rule for
   :ref:`dropping locations<WhenToDropLocation>` should apply here.
 
-* Hoisting identical instructions which appear in several successor blocks into
-  a predecessor block (see ``BranchFolder::HoistCommonCodeInSuccs``). In this
-  case there is no single merged instruction. The rule for
-  :ref:`dropping locations<WhenToDropLocation>` applies.
-
 .. _WhenToDropLocation:
 
 When to drop an instruction location

[SLTozer]

Thanks for looking at this - changes look broadly good (with some inline comments), but we should give some time for debug info code owners to weigh in, in case they have thoughts.

[SLTozer]

Maybe Hoisting/sinking? Particularly relevant since the example, MergedLoadStoreMotion, will hoist loads and sink stores.

[Apochens]

This is a very crucial reminder!

[SLTozer]

I think "merging" should be replaced with "merged". Alternatively as an optional suggestion for the last sentence, "For each group of identical instructions being hoisted/sunk, the merge of all their locations should be applied to the merged instruction."

[dwblaikie]

Is there a way to clarify that this must be from /all/ predecessors or successors (like if you sink something from 2 predecessors into a block that has 3 - you can't use a merged location there, because that makes the location from the first 2 predecessors "reachable" when the 3rd predecessor is taken)

[Apochens]

Actually, the merging updater (eg., applyMergedLocation) will drop the specific row or column number in the debug location when merging different DebugLocs, only keeping their common scope. As a result, the merged debug location would not provide "extra coverage". Moreover, instructions of different predecessors (ie., at least two instructions) are sunk only when they are all identical (for now, I haven't seen any exception). So, I think it's not necessary.

[dwblaikie]

Actually, the merging updater (eg., applyMergedLocation) will drop the specific row or column number in the debug location when merging different DebugLocs, only keeping their common scope. As a result, the merged debug location would not provide "extra coverage".

If both merged locations were the same (perhaps they came from a macro in the source code) - you can end up with bonus coverage if you hoist them, preserve the location (because merging identical locations produces that identical location) but the hoisted path doesn't always lead to at least one of the original paths (eg: because there's a third path the hoisted path can take that skips the merged paths), or came from the same inlined function but are different locations (merging would find the nearest common scope, the inlined function) again, the code would be attributed to the function even though the function may not be reachable.

I figure there might be cases where we could hoist some conditional code into an unconditional path, especially for some code size optimizations - but I don't know of a concrete example. I'll ask around.