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

[Artem Belevich via Phabricator via llvm-commits]

tra added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:65
+  string llvm = "llvm.nvvm.wmma."
+                # frag.geom
+                # "." # op
----------------
timshen wrote:
> 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.
I guess we could implement a join, but I don't think it would improve much. Even your example above is rather hard to read. IMO just linear string concat with `#` is the marginally better as it does not add any further complexity.


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


================
Comment at: llvm/lib/Target/NVPTX/NVPTXIntrinsics.td:7403
+  NVPTXRegClass regclass = !cond(
+    !eq(_ptx_type, "f16") : Float16x2Regs,
+    !eq(_ptx_type, "f32") : Float32Regs);
----------------
timshen wrote:
> It sounds like `_ptx_type` can be reworded to `ptx_element_type`? Also, why the leading underscore?
Done.
`_` was needed to avoid name clash with a field of WMMA_REGS with the same name.



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

https://reviews.llvm.org/D59389