Refine the examples in the debug info document #86272
Conversation
|
@llvm/pr-subscribers-debuginfo Author: Shan Huang (Apochens) ChangesThis 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:
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
requested review from
jryans,
echristo,
adrian-prantl,
SLTozer and
dwblaikie
llvm/docs/HowToUpdateDebugInfo.rst
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
Maybe Hoisting/sinking? Particularly relevant since the example, MergedLoadStoreMotion, will hoist loads and sink stores.
There was a problem hiding this comment.
This is a very crucial reminder!
llvm/docs/HowToUpdateDebugInfo.rst
Outdated
| 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. |
There was a problem hiding this comment.
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."
|
If there is no other concern, could you please approve this PR so that I could merge it after several days. |
| @@ -115,11 +119,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 | |||
There was a problem hiding this comment.
Is there wording we are preserving that covers the case of hoisting/sinking when some but not all successors/predecessors have the instruction? If not, we should probably keep this bullet point and clarify/correct it to refer to this case (of some but not all predecessors/successors)
There was a problem hiding this comment.
Ok, I'll handle this.
llvm/docs/HowToUpdateDebugInfo.rst
Outdated
| * Merging identical loads/stores which occur on both sides of a CFG diamond | ||
| (see the ``MergedLoadStoreMotion`` pass). | ||
| * Hoisting identical instructions from all successors of a conditional branch | ||
| or sinking those from all different predecessors. For example, merging |
There was a problem hiding this comment.
I feel "from all different predecessors" would be better written "from all paths to a postdominating block". The term postdominating should put the reader in mind of the sort of CFG you're describing.
llvm/docs/HowToUpdateDebugInfo.rst
Outdated
| a predecessor block (see ``BranchFolder::HoistCommonCodeInSuccs``). In this | ||
| case there is no single merged instruction. The rule for | ||
| :ref:`dropping locations<WhenToDropLocation>` applies. | ||
| * Hoisting/sinking a part of the identical instructions with the same location. |
There was a problem hiding this comment.
The sentence above reads a bit broken, it's unclear what "a part of the identical instructions" is.
Maybe something like: "Hoisting/sinking that would make a location reachable when it previously wasn't"
There was a problem hiding this comment.
Thanks! This is a good suggestion.
|
If there are no further suggestions regarding the text changes, I plan to merge this PR at the end of the week. Thanks for all your efforts! |
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.