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

[Tony Tye via Phabricator via llvm-commits]

t-tye added inline comments.


================
Comment at: clang/docs/ClangOffloadBundlerFileFormat.rst:11
+
+  <bundle_entry_id>   ::= <offload_kind> "-" <target_triple> "-" <processor_or_target_id>
+
----------------
yaxunl wrote:
> 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. 
My understanding was that some devices do not need a processor to be specified at all. For example, for x86 isn't the target triple enough by itself? In OpenMP it would simply use a bundled code object entry ID that is "opemp-<target-triple>".


================
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>`_).
+
----------------
yaxunl wrote:
> t-tye wrote:
> > (for AMD GPU see :doc:`amdgpu-target-ids`)
> clang documentation is in different URL than llvm documentation.
So LLVM has a mono-repo but the documentation does not have a single TOC tree? That is a shame and rather defeats the benefits of being in a mono repo.


================
Comment at: llvm/docs/AMDGPUUsage.rst:272
 
 Target Features
 ---------------
----------------
yaxunl wrote:
> 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.
My concern is that this patch will obliterate the V3 definition with this V4 one. But I guess you can always reach back to an earlier commit as that is what git is for:-)


================
Comment at: llvm/docs/AMDGPUUsage.rst:2286
                                          for the definition of the mapping.
      ========== ============== ========= =======================================
 
----------------
yaxunl wrote:
> 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?
Sorry I put the comment in the wrong section. I meant to put it in the V3 (not becoming V4) section.


================
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.
 
----------------
yaxunl wrote:
> t-tye wrote:
> > and ``-mcpu``.
> -mcpu is already mentioned by the same sentence
My suggested replacement was to remove the "and those which specify target features." since those options are being removed. So the "and" needs to go before the -mcpu to become:

```
Used by the assembler to validate command-line options such as ``-triple`` and ``-mcpu``.
```


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

https://reviews.llvm.org/D84822