[PATCH] D81198: [docs] Specify rules for updating debug locations

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Thanks! I added a few nits inline.



================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:35
+The purpose of this rule is to ensure that common block-local optimizations
+preserve the ability to set breakpoints on source lines corresponding to the
+instructions they touch. Debugging, as well as SamplePGO accuracy, would be
----------------
s/source lines/source locations/

That's also what they are called in Clang.


================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:36
+preserve the ability to set breakpoints on source lines corresponding to the
+instructions they touch. Debugging, as well as SamplePGO accuracy, would be
+severely impacted if that ability were lost.
----------------
`Debugging, crash logs, and SamplePGO accuracy, ...`


================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:41
+
+* Instruction scheduling. Block-local instruction reordering should not drop line
+  locations, even though this may lead to jumpy single-stepping behavior.
----------------
s/line/source/


================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:46
+  ``B2``, *and* is its unique predecessor, instructions from ``B2`` can be
+  hoisted into ``B1``. Line locations from ``B2`` should be preserved.
+
----------------
Source


================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:48
+
+* Simple peephole optimizations, like ``(X + X) => (X << 1)``. The location of
+  the ``shl`` instruction should be the same as the location of the ``add``
----------------
Simple peephole optimizations that replace or expand an instruction, like


================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:49
+* Simple peephole optimizations, like ``(X + X) => (X << 1)``. The location of
+  the ``shl`` instruction should be the same as the location of the ``add``
+  instruction.
----------------
I would either reuse + and << or use add and shl in the previous line


================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:54
+  unconditionally branch to ``B3`` and ``B3`` can be folded into its
+  predecessors, breakpoint locations from ``B3`` should be preserved.
+
----------------
s/breakpoint/source/


================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:88
+
+* Complex peephole optimizations which combine multiple instructions together,
+  like ``(A * B) + C => llvm.fma.f32(A, B, C)``.  Note that the location of the
----------------
I wonder if we should drop the Simple/Complex adjectives altogether?


================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:97
+  ``(sext (zext x)) => (zext x)``. The rule for
+  :ref:`preserving locations<WhenToPreserveLocation>` should apply here.
+
----------------
I would be curious about an explanation what makes this example different from the fma example above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81198/new/

https://reviews.llvm.org/D81198