[llvm] [RFC][AMDGPU][Doc] Add developer guideline (PR #194425)

[Shilei Tian via llvm-commits]

================
@@ -0,0 +1,405 @@
+===============================
+Developer Guideline for AMDGPU
+===============================
+
+.. contents::
+   :local:
+
+Introduction
+============
+
+This document highlights coding conventions, test policies, and other
+development guidelines that apply to all AMDGPU-related code across the LLVM
+project (the backend in ``llvm/lib/Target/AMDGPU``, Clang AMDGPU support, LLD,
+associated tests, etc.).  It is **not** a replacement for or summary of the
+`LLVM Coding Standards <https://llvm.org/docs/CodingStandards.html>`_ or the
+`LLVM Testing Guide <https://llvm.org/docs/TestingGuide.html>`_; contributors
+are expected to be familiar with those documents as well.
+
+The topics covered here are those that come up frequently during AMDGPU code
+reviews.  Some overlap with existing upstream rules and are restated here for
+easy reference.
+
+Coding Standards
+================
+
+AMDGPU-related code follows the
+`LLVM Coding Standards <https://llvm.org/docs/CodingStandards.html>`_ with the
+refinements listed below.
+
+Use of ``auto``
+---------------
+
+The LLVM Coding Standards describe the policy for ``auto`` in
+`Use auto Type Deduction to Make Code More Readable <https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable>`_.
+Below are more concrete examples of how that policy applies in AMDGPU code.
+
+Do **not** use ``auto`` except in the following cases:
+
+* Lambda expressions:
+
+  .. code-block:: c++
+
+     auto Pred = [](unsigned Val) { return Val > 0; };
+
+* Casts where the target type is already spelled out on the right-hand side
+  (``cast``, ``dyn_cast``, ``static_cast``, etc.):
+
+  .. code-block:: c++
+
+     auto *Inst = cast<CallInst>(V);
+     auto *MD = dyn_cast<MDNode>(Op);
+     auto Width = static_cast<unsigned>(Val);
+
+* Iterators:
+
+  .. code-block:: c++
+
+     auto It = Container.begin();
+
+* Structured bindings:
+
+  .. code-block:: c++
+
+     auto [Key, Value] = Pair;
+
+In all other cases, write the type explicitly.
+
+.. code-block:: c++
+
+   // Avoid - the type is not obvious from the right-hand side.
+   auto Reg = MI.getOperand(0).getReg();
+   auto Size = DL.getTypeAllocSize(Ty);
+
+   // Preferred - spell out the type.
+   Register Reg = MI.getOperand(0).getReg();
+   TypeSize Size = DL.getTypeAllocSize(Ty);
+
+Use of Braces
+-------------
+
+The LLVM Coding Standards discuss brace usage in
+`Don't Use Braces on Simple Single-Statement Bodies of if/else/loop Statements <https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements>`_.
+In AMDGPU code, braces may be omitted **only when the single statement
+fits on one line**.  If the statement spans more than one line (e.g. because of
+a long argument list that wraps), keep the braces.
+
+For ``if``/``else`` chains, if **either** branch requires braces, add braces to
+**all** branches to keep them symmetric.
+
+.. code-block:: c++
+
+   // OK - single statement on one line, braces omitted.
+   for (unsigned I = 0; I < N; ++I)
+     doSomething(I);
+
+   // Required - single statement, but spans multiple lines.
+   if (Cond) {
+     doSomethingElse(LongArgument1,
+                     LongArgument2);
+   }
+
+   // Required - the else branch needs braces, so the if branch gets them too.
+   if (Cond) {
+     doSomething();
+   } else {
+     doSomethingElse(LongArgument1,
+                     LongArgument2);
+   }
+
+   // Avoid - asymmetric braces.
+   if (Cond)
+     doSomething();
+   else {
+     doSomethingElse(LongArgument1,
+                     LongArgument2);
+   }
+
+Instruction Naming
+------------------
+
+Instruction names in TableGen definitions (e.g. in ``VOP3PInstructions.td``,
+``VOP2Instructions.td``, etc.) should follow the terminology of the ISA
+documentation.  Use **all-caps** names that match the documentation's name for
+at least one target.  When the compiler needs a variant of a documented
+instruction (e.g. a version with additional property flags or extra register
+uses), append a **lowercase** suffix to distinguish it from the canonical name.
+
+.. code-block:: text
+
+   // Good - matches the ISA documentation exactly.
+   defm V_ADD_F32 : VOP2Inst_VOPD <"v_add_f32", ...>;
+
+   // Good - lowercase suffix for a compiler-invented variant.
+   defm V_ADD_F32_e64 : ...;
+
+   // Avoid - deviates from the documented name without reason.
+   defm V_Add_F32 : ...;
+
+   // Avoid - all-caps suffix for a compiler-invented variant;
+   // use a lowercase suffix instead.
+   defm V_ADD_F32_E64 : ...;
+
+Error and Diagnostic Messages
+-----------------------------
+
+Messages passed to ``assert``, ``llvm_unreachable``, ``report_fatal_error``,
+diagnostic handlers, and similar should **not** start with an uppercase letter.
+Use lowercase as if the message were a continuation of a sentence, not the
+beginning of one.  Do not end the message with a period.
+
+.. code-block:: c++
+
+   // Good
+   report_fatal_error("malformed block");
+
+   // Avoid
+   report_fatal_error("Malformed block");
+
+Design Practices
+=================
----------------
shiltian wrote:

> I suppose usage of values such as llvm::AMDGPUAS::FLAT_ADDRESS rather than the addrspace constant.

I'd say this is more of a general C++ best practice than something LLVM-specific.

> Since the AMDGPU backend is a "GPU backend", I've often found that thinking about uniformity and divergence is very useful for understanding the broader implications of the code, testing SGPR/VGPR inputs, and so on. I don't know whether it belongs in a "developer guideline", but definitely in a "developer checklist". I would like a mention of it.

Hmm, I don't really feel this fits into this guideline, and "think about" isn't really an actionable item.

> Are we waiting on including some of this in the broader LLVM developer guidelines, or are we still pushing to get this in despite the redundancy?

I still hope to get this in, and the redundancy is actually intentional to emphasize these points, since we've repeatedly brought them up during reviews.

https://github.com/llvm/llvm-project/pull/194425