[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:
> 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>".
fixed


================
Comment at: llvm/docs/AMDGPUUsage.rst:2286
                                          for the definition of the mapping.
      ========== ============== ========= =======================================
 
----------------
t-tye wrote:
> 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.
Moved to V3.


================
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:
> 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``.
> ```
Sorry, I misunderstood. Fixed.


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

https://reviews.llvm.org/D84822