[PATCH] D115587: [AMDGPU][NFC] Add documentation for location description DWARF extension

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

LGTM, with only typos and minor nits around wording.

I'm OK with you landing this as-is, and addressing these post-commit, if you prefer.



================
Comment at: llvm/docs/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack.md:33
+This extension is to generalize the DWARF expression evaluation model to allow
+location descriptions to be manipulated on the stack. It is done in manner that
+is backwards compatible with DWARF 5. This permits operations to act on location
----------------



================
Comment at: llvm/docs/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack.md:65
+memory accesses which tend to be more expensive than on a CPU due to the much
+large number of threads concurrently executing. In addition to traditional
+scalar register of a CPU, these devices often have many wide vector registers.
----------------



================
Comment at: llvm/docs/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack.md:66
+large number of threads concurrently executing. In addition to traditional
+scalar register of a CPU, these devices often have many wide vector registers.
+
----------------



================
Comment at: llvm/docs/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack.md:110
+
+DWARF maps source program language entities to the hardware representation. For
+example:
----------------



================
Comment at: llvm/docs/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack.md:110
+
+DWARF maps source program language entities to the hardware representation. For
+example:
----------------
scott.linder wrote:
> 
Nit: As the mapping is in both directions, maybe something like "DWARF maps between source program language entities and their hardware representations"?


================
Comment at: llvm/docs/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack.md:130
+- At a memory address.
+- At an offset form the current stack pointer.
+- Optimized away, but with a known compiler time value.
----------------



================
Comment at: llvm/docs/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack.md:382-383
+
+Some features only work when located in global memory. The type attribute
+expressions requiring a base object which could be in any kind of location.
+
----------------
I have a hard time reading this, I'm still not sure I quite follow it. I assume the intent is to describe how the "base object" feature in DWARF expressions only works if the object is in global memory?


================
Comment at: llvm/docs/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack.md:394
+
+CFI location expression do not allow composite locations or non-global address
+space memory locations. Both these are needed in optimized code for devices with
----------------



================
Comment at: llvm/docs/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack.md:426
+  the size of that specific register.
+- For an implict, it is the linear stream of bytes of the value when represented
+  using the value's base type which specifies the encoding, size, and byte
----------------



================
Comment at: llvm/docs/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack.md:622
+This example is the same as the previous one, except the first 2 bytes of the
+second vector register has been spilled to memory, and the last 2 bytes have been proven to be a constant and optimized away.
+
----------------



================
Comment at: llvm/docs/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack.md:715
+number. Each address space is associated with a separate storage. A memory
+location description is pushed which refers to the address spaces' storage, with
+an offset of the popped value.
----------------



================
Comment at: llvm/docs/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack.md:742
+Since DW_OP_piece now takes any kind of location description for its pieces, it
+is now possible to have parts of a composite to involve locations in different
+address spaces. For example, this can happen when parts of a source variable
----------------



================
Comment at: llvm/docs/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack.md:765
+
+![Bit Offsets Example](images/10-extenson-bit-offset.example.png)
+
----------------
Nit: this is very minor, but `extenson` should be `extension` in these filenames


================
Comment at: llvm/docs/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack.md:781
+
+![Bit Offsets Example: Step 4](images/10-extenson-bit-offset.example.frame.1.png)
+
----------------



================
Comment at: llvm/docs/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack.md:794
+spill registers to parts of other registers, to non-global memory address
+spaces, or even a composite ot different location kinds.
+
----------------



================
Comment at: llvm/docs/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack.md:856
+effective PC of lanes of SIMT execution. The vector result efficiently computes
+the PC for each SMPT lane at once. The mask could be the hardware execution mask
+register that controls which SIMT lanes are executing. For active divergent
----------------



================
Comment at: llvm/docs/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack.md:877
+Effectively, during that range, the source variable is in both memory and a
+register. If consumer, such as a debugger, allows the user to change the value
+of the source variable in that PC range, then it would need to change both
----------------



================
Comment at: llvm/docs/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack/AMDGPUDwarfExtensionAllowLocationDescriptionOnTheDwarfExpressionStack.md:899
+location description, or a DWARF operation expression that uses the
+DW_OP_push_object_address operation, my want to act on the result of another
+expression that returned a location description involving multiple places.
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115587