[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.
This revision is now accepted and ready to land.


================
Comment at: llvm/trunk/docs/AMDGPUUsage.rst:3797
+
+Each hardware stage has a set of 32-bit *user data registers* which can be
+written from a command buffer and then loaded into SGPRs when waves are launched
----------------
Do all generations support 32 user SGPRs or was this increased to 32 in GFX9? Do compute shaders still only support 16 use SGPRs?


================
Comment at: llvm/trunk/docs/AMDGPUUsage.rst:3816
+     ============= ================================
+     User Register Description
+     ============= ================================
----------------
Are these SGPR register numbers? If so suggest changing header to say that.


================
Comment at: llvm/trunk/docs/AMDGPUUsage.rst:3823
+     13 - 14       Thread Group Count (64-bit pointer)
+     15            GDS Range
+     ============= ================================
----------------
Could this be defined?


================
Comment at: llvm/trunk/docs/AMDGPUUsage.rst:3838-3844
+     +             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)
+     +             Vertex Offset (First Stage Only)
+     +             Instance Offset (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)
     ============= ================================
```


================
Comment at: llvm/trunk/docs/AMDGPUUsage.rst:3842-3844
+     +             Draw Index (First Stage Only)
+     +             Vertex Offset (First Stage Only)
+     +             Instance Offset (First Stage Only)
----------------
Could these be defined?


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


```
.. _amdpal_global_internal_table:
```


================
Comment at: llvm/trunk/docs/AMDGPUUsage.rst:3902
+  counter's top 32 bits.
+
 Unspecified OS
----------------
Could there be sections to define the other structures mentioned?

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


================
Comment at: llvm/trunk/docs/AMDGPUUsage.rst:3902
+  counter's top 32 bits.
+
 Unspecified OS
----------------
t-tye wrote:
> Could there be sections to define the other structures mentioned?
> 
> - Per-Shader Internal Table (32-bit pointer)
> - Spill Table (32-bit pointer)
Could there be a section to define the metadata and how it specifies how these structures are set up?


Repository:
  rL LLVM

https://reviews.llvm.org/D45246