[PATCH] D59389: [NVPTX] Refactor generation of MMA intrinsics and instructions. NFC.

[Tim Shen via Phabricator via llvm-commits]

timshen added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:46
+// Helper class that represents a 'fragment' of an NVPTX *MMA instruction.
+class WMMA_REGS<string _geom, string _frag, string _ptx_type> {
+  string geom = _geom;
----------------
What's _geom / _frag / _ptx_type? Maybe have some examples, or a comment like one of these down below:
`wmma.load.[a|b|c].sync.[row|col].m16n16k16[|.global|.shared].[f16|f32]`
or maybe a regex in the comment?


================
Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:64
+class WMMA_NAME_LDST<string op, WMMA_REGS frag, string Layout, int WithStride> {
+  string llvm = "llvm.nvvm.wmma."
+                # frag.geom
----------------
why `string llvm`? Maybe string `intrinsic`?


================
Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:65
+  string llvm = "llvm.nvvm.wmma."
+                # frag.geom
+                # "." # op
----------------
How easy it is to write a string join function? For example:
```
!join(".", ["llvm", "nvvm", "wmma", "op", frag.frag, Layout] +
    !if(WithStride, [".stride"], []) +
    [frag.ptx_type])
```

It is slightly more readable and slightly higher level.

Ditto to other string joins.


================
Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:72
+                ;
+  // TODO(tra@): record name should ideally use the same field order as the intrinsic.
+  // E.g. string record = !subst("llvm", "int",
----------------
Usually people use `TODO(tra)`.


================
Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:74
+  // E.g. string record = !subst("llvm", "int",
+  //                      !subst(".", "_", llvm));
+  string record = "int_nvvm_wmma_"
----------------
!subst(".", "_",
!subst("llvm.", "int_", llvm))?

The difference is that it has a more specific match on "llvm.", instead of "llvm". Only if we can match regex "^llvm\."...


================
Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:84
+
+class WMMA_NAME_MMA<string ALayout, string BLayout,
+                    WMMA_REGS c, WMMA_REGS d,
----------------
For the parameter names, why is CamelCase mixed with snake_case?


================
Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:96
+
+  string record = !subst("llvm", "int",
+                  !subst(".", "_", llvm));
----------------
ditto.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXIntrinsics.td:7389
+// Generates list of n sequential register names.
+class RegNames<int n, string prefix> {
+  list<string> ret = !if(n, !listconcat(RegNames<!add(n,-1), prefix>.ret,
----------------
`class RegSeq`?


================
Comment at: llvm/lib/Target/NVPTX/NVPTXIntrinsics.td:7403
+  NVPTXRegClass regclass = !cond(
+    !eq(_ptx_type, "f16") : Float16x2Regs,
+    !eq(_ptx_type, "f32") : Float32Regs);
----------------
It sounds like `_ptx_type` can be reworded to `ptx_element_type`? Also, why the leading underscore?


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

https://reviews.llvm.org/D59389