[NFC][AMDGPU] Document target ID syntax for code object V2 to V3

[kzhuravl]

LGTM

[MaskRay]

In your commit the message does not include Reviewed by:. Many people agree that both Reviewed by: & Differential Revision: should be present. The Reviewed by: list indicates people who acknowledged the patch. (The Reviewers: list does not necessarily mean all the people on the list have acknowledged the patch so Reviewers: is mostly useless.)

arc amend can fetch the Phabricator summary and amend the local description.

You can install llvm/utils/git/pre-push.py to prevent accidental Summary:, Reviewers:, Subscribers: and Tags: in the presence of Differential Revision:.

[scott.linder]

In D95018#2523019, @MaskRay wrote:

In your commit the message does not include Reviewed by:. Many people agree that both Reviewed by: & Differential Revision: should be present. The Reviewed by: list indicates people who acknowledged the patch. (The Reviewers: list does not necessarily mean all the people on the list have acknowledged the patch so Reviewers: is mostly useless.)

arc amend can fetch the Phabricator summary and amend the local description.

You can install llvm/utils/git/pre-push.py to prevent accidental Summary:, Reviewers:, Subscribers: and Tags: in the presence of Differential Revision:.

Why is including anything besides "Differential Revision:" being permitted/required? It seems unfortunate enough that anything is needed in the commit message at all, but at least it is a single, relatively unobtrusive thing. "Denormalizing" the message by duplicating information contained in the Differential Revision doesn't seem helpful.

If the utility is in avoiding the need to query Phabricator to get this information, then can we make the "Reviewed By:" field include full names, rather than (often opaque) usernames/handles?

[MaskRay]

In D95018#2523615, @scott.linder wrote:

In D95018#2523019, @MaskRay wrote:

In your commit the message does not include Reviewed by:. Many people agree that both Reviewed by: & Differential Revision: should be present. The Reviewed by: list indicates people who acknowledged the patch. (The Reviewers: list does not necessarily mean all the people on the list have acknowledged the patch so Reviewers: is mostly useless.)

arc amend can fetch the Phabricator summary and amend the local description.

You can install llvm/utils/git/pre-push.py to prevent accidental Summary:, Reviewers:, Subscribers: and Tags: in the presence of Differential Revision:.

Why is including anything besides "Differential Revision:" being permitted/required? It seems unfortunate enough that anything is needed in the commit message at all, but at least it is a single, relatively unobtrusive thing. "Denormalizing" the message by duplicating information contained in the Differential Revision doesn't seem helpful.

If the utility is in avoiding the need to query Phabricator to get this information, then can we make the "Reviewed By:" field include full names, rather than (often opaque) usernames/handles?

Sorry, the consensus is that Differential Revision: is needed. Reviewed by: is optional. Others are not useful.

[scott.linder]

In D95018#2523640, @MaskRay wrote:

In D95018#2523615, @scott.linder wrote:

In D95018#2523019, @MaskRay wrote:

In your commit the message does not include Reviewed by:. Many people agree that both Reviewed by: & Differential Revision: should be present. The Reviewed by: list indicates people who acknowledged the patch. (The Reviewers: list does not necessarily mean all the people on the list have acknowledged the patch so Reviewers: is mostly useless.)

arc amend can fetch the Phabricator summary and amend the local description.

You can install llvm/utils/git/pre-push.py to prevent accidental Summary:, Reviewers:, Subscribers: and Tags: in the presence of Differential Revision:.

Why is including anything besides "Differential Revision:" being permitted/required? It seems unfortunate enough that anything is needed in the commit message at all, but at least it is a single, relatively unobtrusive thing. "Denormalizing" the message by duplicating information contained in the Differential Revision doesn't seem helpful.

If the utility is in avoiding the need to query Phabricator to get this information, then can we make the "Reviewed By:" field include full names, rather than (often opaque) usernames/handles?

Sorry, the consensus is that Differential Revision: is needed. Reviewed by: is optional. Others are not useful.

Having an optional field that people are reminded to always include seems to imply it should just be required, but I guess it is a minor point. I'll just try to remember to include the "Reviewed By:" in the future. Does arc land work with our repo, and if so will it always update the message to the canonical one automatically?

[MaskRay]

In D95018#2523721, @scott.linder wrote:

In D95018#2523640, @MaskRay wrote:

In D95018#2523615, @scott.linder wrote:

In D95018#2523019, @MaskRay wrote:

In your commit the message does not include Reviewed by:. Many people agree that both Reviewed by: & Differential Revision: should be present. The Reviewed by: list indicates people who acknowledged the patch. (The Reviewers: list does not necessarily mean all the people on the list have acknowledged the patch so Reviewers: is mostly useless.)

arc amend can fetch the Phabricator summary and amend the local description.

You can install llvm/utils/git/pre-push.py to prevent accidental Summary:, Reviewers:, Subscribers: and Tags: in the presence of Differential Revision:.

Why is including anything besides "Differential Revision:" being permitted/required? It seems unfortunate enough that anything is needed in the commit message at all, but at least it is a single, relatively unobtrusive thing. "Denormalizing" the message by duplicating information contained in the Differential Revision doesn't seem helpful.

If the utility is in avoiding the need to query Phabricator to get this information, then can we make the "Reviewed By:" field include full names, rather than (often opaque) usernames/handles?

Sorry, the consensus is that Differential Revision: is needed. Reviewed by: is optional. Others are not useful.

Having an optional field that people are reminded to always include seems to imply it should just be required, but I guess it is a minor point. I'll just try to remember to include the "Reviewed By:" in the future. Does arc land work with our repo, and if so will it always update the message to the canonical one automatically?

arc amend definitely keeps just "Reviewed by:" and Differential Revision:. I don't know arc land as I don't use it much.

(Personally I run git pull --rebase origin main && git commit --amend --date=now --no-edit && git push origin HEAD:main. The --date=now amend is to reset the author date to committer date.)

llvm/docs/AMDGPUUsage.rst

**processor**
   Is a AMDGPU processor or alternative processor name specified in
   :ref:`amdgpu-processor-table`. The non-canonical form target ID allows both
   the primary processor and alternative processor names. The canonical form
   target ID only allow the primary processor name.
 
 **target-feature**
   Is a target feature name specified in :ref:`amdgpu-target-features-table` that
   is supported by the processor. The target features supported by each processor
-  is specified in :ref:`amdgpu-processor-table`. Those that can be specifeid in
+  is specified in :ref:`amdgpu-processor-table`. Those that can be specified in
   a target ID are marked as being controlled by ``-mcpu`` and
   ``--offload-arch``. Each target feature must appear at most once in a target
   ID. The non-canonical form target ID allows the target features to be
   specified in any order. The canonical form target ID requires the target
   features to be specified in alphabetic order.
 
+.. _amdgpu-target-id-v2-v3:
+
+Code Object V2 to V3 Target ID
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The target ID syntax for code object V2 to V3 is the same as defined in `Clang
+Offload Bundler <https://clang.llvm.org/docs/ClangOffloadBundler.html>`_ except
+when used in the :ref:`amdgpu-assembler-directive-amdgcn-target` assembler
+directive and the bundle entry ID. In those cases it has the following BNF
+syntax:
+
+.. code::
+
+  <target-id> ::== <processor> ( "+" <target-feature> )*
+
+Where a target feature is omitted if *Off* and present if *On* or *Any*.
+
+.. note::
+
+  The code object V2 to V3 cannot represent *Any* and treats it the same as
+  *On*.
+
 .. _amdgpu-embedding-bundled-objects:
 
 Embedding Bundled Code Objects
 ------------------------------
 
 AMDGPU supports the HIP and OpenMP languages that perform code object embedding
 as described in `Clang Offload Bundler
 <https://clang.llvm.org/docs/ClangOffloadBundler.html>`_.
 
+.. note::
+
+  The target ID syntax used for code object V2 to V3 for a bundle entry ID
+  differs from that used elsewhere. See :ref:`amdgpu-target-id-v2-v3`.
+
 .. _amdgpu-address-spaces:
 
 Address Spaces
 --------------
 
 The AMDGPU architecture supports a number of memory address spaces. The address
 space names use the OpenCL standard names, with some additions.
 

 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 Directives which begin with ``.amdgcn`` are valid for all ``amdgcn``
 architecture processors, and are not OS-specific. Directives which begin with
 ``.amdhsa`` are specific to ``amdgcn`` architecture processors when the
 ``amdhsa`` OS is specified. See :ref:`amdgpu-target-triples` and
 :ref:`amdgpu-processors`.
 
+.. _amdgpu-assembler-directive-amdgcn-target:
+
 .amdgcn_target <target-triple> "-" <target-id>
 ++++++++++++++++++++++++++++++++++++++++++++++
 
 Optional directive which declares the ``<target-triple>-<target-id>`` supported
 by the containing assembler source file. Used by the assembler to validate
 command-line options such as ``-triple``, ``-mcpu``, and
 ``--offload-arch=<target-id>``. A non-canonical target ID is allowed. See
 :ref:`amdgpu-target-triples` and :ref:`amdgpu-target-id`.
 
+.. note::
+
+  The target ID syntax used for code object V2 to V3 for this directive differs
+  from that used elsewhere. See :ref:`amdgpu-target-id-v2-v3`.
+
 .amdhsa_kernel <name>
 +++++++++++++++++++++
 
 Creates a correctly aligned AMDHSA kernel descriptor and a symbol,
 ``<name>.kd``, in the current location of the current section. Only valid when
 the OS is ``amdhsa``. ``<name>`` must be a symbol that labels the first
 instruction to execute, and does not need to be previously defined.