[PATCH] D84822: Add documentation for target ID and ClangOffloadBundlerFormat

[Yaxun Liu via Phabricator via llvm-commits]

yaxunl added inline comments.


================
Comment at: clang/docs/ClangOffloadBundlerFileFormat.rst:11
+
+  <bundle_entry_id>   ::= <offload_kind> "-" <target_triple> "-" <processor_or_target_id>
+
----------------
t-tye wrote:
> [  "-" <processor_or_target_id> ]
> 
> This is optional as some targets do not support this.
If a target does not support target ID, <processor_or_target_id> will be processor, so this item is still there. 


================
Comment at: clang/docs/ClangOffloadBundlerFileFormat.rst:25
+  binary is compiled. If target ID is supported by the target, this is the target ID for which the
+  device binary is compiled (see `Target ID Definition <https://llvm.org/docs/AMDGPUUsage.html#target-ids>`_).
+
----------------
t-tye wrote:
> (for AMD GPU see :doc:`amdgpu-target-ids`)
clang documentation is in different URL than llvm documentation.


================
Comment at: llvm/docs/AMDGPUUsage.rst:227
+                                                                                       names.
+     ``gfx906``                  ``amdgcn``   dGPU  - sram-ecc                      - Radeon Instinct MI50
+                                                    - xnack                         - Radeon Instinct MI60
----------------
t-tye wrote:
> Change all occurences of sram-ecc to sramecc to avoid problems that hyphen is used as a bundled code object entry ID separator.
done


================
Comment at: llvm/docs/AMDGPUUsage.rst:272
 
 Target Features
 ---------------
----------------
t-tye wrote:
> We should move the code object V3 into a separate page so we continue to define that ABI since old code objects still exist. Then these changes can define the code object V4.
can we do that with a separate patch? This patch already contains too many changes.


================
Comment at: llvm/docs/AMDGPUUsage.rst:2286
                                          for the definition of the mapping.
      ========== ============== ========= =======================================
 
----------------
t-tye wrote:
> t-tye wrote:
> > Add "TargetID" attribute that is a string that is the target ID for the module.
> Or is this:
> 
> <target_triple> "-" <target_id>
This is V2. Are we sure we want to add target ID to V2?


================
Comment at: llvm/docs/AMDGPUUsage.rst:7683-7684
 :ref:`amdgpu-amdhsa-code-object-target-identification`. Used by the assembler
 to validate command-line options such as ``-triple``, ``-mcpu``, and those
 which specify target features.
 
----------------
t-tye wrote:
> and ``-mcpu``.
-mcpu is already mentioned by the same sentence


================
Comment at: llvm/docs/AMDGPUUsage.rst:7760
                                                                                                :ref:`amdgpu-amdhsa-compute_pgm_rsrc1-gfx6-gfx10-table`.
      ``.amdhsa_reserve_xnack_mask``                           Target              GFX8-GFX10   Whether the kernel may trigger XNACK replay.
                                                               Feature                          Used to calculate GRANULATED_WAVEFRONT_SGPR_COUNT in
----------------
t-tye wrote:
> This directive is being removed since the information is now present in the target ID that is provided by the .amdgcn_target directive.
done


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

https://reviews.llvm.org/D84822