[PATCH] D138869: [Docs][RFC] Add AMDGPU LLVM Extensions for Heterogeneous Debugging

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Hi Scott,

I really enjoyed the conference talk about this, and moving the issues of how variables are fragmented into smaller chunks to higher up in the metadata hierachy makes a lot of sense. It could substantially simplify + improve our tracking of variables today,

There's a lot of different things being re-engineered in this proposal, and I'd like to make sure that I have the correct understanding of how the current variable location design maps to this new one. As I understand it, dbg.values become dbg.def and dbg.kill intrinsics, connecting IR Values to DILifeime objects. The DILifetimes refer to a hierarchy of DIFragments that specify what's being defined (which is great), and the expression required to produce the variable value / location from the inputs.

After that it becomes fuzzier though: it's not obvious to me how the current variable value / location is determined when there can be (according to the document) multiple disjoint and overlapping lifetimes that are active. If a variable fragment has different runtime values, which one should we pick, and how -- or if they're supposed to always have the same value, what guarantees this during optimisation? Right now, dbg.value intrinsics are effectively an assignment to the variable [fragment], and the variable value is the last dominating dbg.value assignment (or possibly a PHI between multiple of them, determined by LiveDebugValues). What is the equivalent for these new intrinsics?

I think lifetimes and def/kill relationships makes sense after register allocation where that's the form the program is in, but it's not clear how it would work in SSA form. It's also worth noting that the multiple-locations-after-regalloc problem is solved, to a large extent, by the instruction referencing rewrite [0], essentially keeping the debugging information in SSA form and then recognising target COPY and value-movements to track the multiple locations a value can be resident.

There's value in having multiple ways of expressing variable locations, during loop-strength-reduction you can recompute a variable from the loop starting values or from the strength-reduced variables, for example. It needs to be approached with some delicacy though to save memory.

At a more abstract level, I've a worry that this might move us more in the direction of requiring more knowledge / maintenence during optimisation passes to preserve debug-info invariants, where it seems more beneficial to reduce that kind of maintenence, in compile and engineering time. It's certainly the motivation behind the assignment tracking work [1], which is inferring information about optimisations from what gets deleted rather than what gets preserved.

[0] https://www.youtube.com/watch?v=yxuwfNnp064
[1] https://discourse.llvm.org/t/rfc-assignment-tracking-a-better-way-of-specifying-variable-locations-in-ir/62367



================
Comment at: llvm/docs/AMDGPULLVMExtensionsForHeterogeneousDebugging.rst:2041-2045
+This seems to be supported by the fact that even in current LLVM trunk, with the
+more conservative change to mark the ``redundant`` variable as ``undef`` in the
+above case, changing the source to modify ``redundant`` after the load results
+in both ``redundant`` and ``loaded`` referring to the same location, and both
+being read-write. A modification of ``redundant`` in the debugger before the use
----------------
This is because `redundant` isn't trivially dead after this modification, causing the load to be CSE'd, which causes a RAUW of the `Value` that keeps the dbg.value alive. We could achieve the same results in the unmodified case by searching the function for equivalent `Value`s whenever we delete trivially dead code and need to salvage variable locations, but it would be compile-time expensive.


================
Comment at: llvm/docs/AMDGPULLVMExtensionsForHeterogeneousDebugging.rst:2049-2073
+.. code:: c
+   :number-lines:
+
+   int
+   foo(int *bar, int arg, int more)
+   {
+     int redundant = *bar;
----------------
Note that with the modification, a value is loaded from `*bar` and then stored back to `*bar`, which EarlyCSE successfully spots as being redundant, and deletes the heap store, which was the primary problem in PR40628.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138869