[PATCH] D45246: Add AMDPAL Code Conventions section to AMD docs

[Tony Tye via Phabricator via llvm-commits]

t-tye reopened this revision.
t-tye added inline comments.


================
Comment at: llvm/docs/AMDGPUUsage.rst:3808-3810
+Note that there are always 10 available *user data entries* in registers -
+entries beyond that limit must be fetched from memory (via the spill table
+pointer) by the shader.
----------------
Clarify that this is User SGPR Registers. Also should this define the System SGPR and VGPR Registers.


================
Comment at: llvm/docs/AMDGPUUsage.rst:3816
+     ============= ================================
+     User Register Description
+     ============= ================================
----------------
User SGPR Registers


================
Comment at: llvm/docs/AMDGPUUsage.rst:3829
+
+Graphics pipelines support a much more flexible user data mapping:
+
----------------
Add System SGPR and VGPR mapping information.


================
Comment at: llvm/docs/AMDGPUUsage.rst:3835
+     ============= ================================
+     User Register Description
+     ============= ================================
----------------
User SGPR Registers


================
Comment at: llvm/docs/AMDGPUUsage.rst:3836-3845
+     ============= ================================
+     0             Global Internal Table (32-bit pointer)
+     +             Per-Shader Internal Table (32-bit pointer)
+     + 1-15        Application Controlled User Data
+                   (1-15 Contiguous 32-bit Values in Registers)
+     +             Spill Table (32-bit pointer)
+     +             Draw Index (First Stage Only)
----------------
Need to remove the "+" as that is making a bulleted list. I think this is what you want which puts a list of the possible values in the 1-15 table row:

============= ================================
User Register Description
============= ================================
0             :ref:`amdpal_global_internal_table` (32-bit pointer)
1-15          Application Controlled User Data
              (1-15 Contiguous 32-bit Values in Registers)

              - Per-Shader Internal Table (32-bit pointer)
              - Spill Table (32-bit pointer)
              - Draw Index (First Stage Only)
              - Vertex Offset (First Stage Only)
              - Instance Offset (First Stage Only)
============= ================================

Define these fields and how the metadata sets them up.


================
Comment at: llvm/docs/AMDGPUUsage.rst:3858-3862
+  * The application-controlled user data range supports compaction remapping, so
+    only *entries* that are actually consumed by the shader must be assigned to
+    corresponding *registers*. Note that in order to support an efficient runtime
+    implementation, the remapping must pack *registers* in the same order as
+    *entries*, with unused *entries* removed.
----------------
Define how the mapping and re-mapping is conveyed and what the rules are. From tjis description I know there is a mapping but am unclear on how it is expressed.


================
Comment at: llvm/docs/AMDGPUUsage.rst:3864
+
+.. _pal_global_internal_table:
+
----------------
To be consistent with the section name:

.. _amdpal_global_internal_table:


================
Comment at: llvm/docs/AMDGPUUsage.rst:3870
+The global internal table is a table of *shader resource descriptors* (SRDs) that
+define how certain engine-wide, runtime-managed resources should be accessed
+from a shader. The majority of these resources have HW-defined formats, and it
----------------
Where is the concept of an "engine" defined? Is this just another term for the PAL runtime? If so I would stick with saying PAL runtime. I would clarify use of runtime with PAL Runtime since the higher levels also have runtimes.


================
Comment at: llvm/docs/AMDGPUUsage.rst:3872
+from a shader. The majority of these resources have HW-defined formats, and it
+is up to the compiler to write/read data as required by the target hardware.
+
----------------
Would be helpful to reference where the target hardware format is defined.


================
Comment at: llvm/docs/AMDGPUUsage.rst:3879-3896
+     ============= ================================
+     Offset        Description
+     ============= ================================
+     0-3           Graphics Scratch SRD
+     4-7           Compute Scratch SRD
+     8-11          ES/GS Ring Output SRD
+     12-15         ES/GS Ring Input SRD
----------------
Add sections to define the other structures mentioned:

    Per-Shader Internal Table (32-bit pointer)
    Spill Table (32-bit pointer)

Section to define the metadata and how it specifies how these structures are set up.


================
Comment at: llvm/docs/AMDGPUUsage.rst:3898-3901
+  The pointer to the global internal table passed to the shader as user data
+  is a 32-bit pointer. The top 32 bits should be assumed to be the same as
+  the top 32 bits of the pipeline, so the shader may use the program
+  counter's top 32 bits.
----------------
Suggest being more explicit. What does "top 32 bits should be assumed to be the same as
  the top 32 bits of the pipeline" mean? Presumably it is the loaded PC address of the code for the shader pipeline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D45246