[PATCH] D38645: [NVPTX] Implemented wmma intrinsics and instructions.

[Artem Belevich via Phabricator via llvm-commits]

tra added a comment.

In https://reviews.llvm.org/D38645#895049, @YuanLin wrote:

> We took this approach to reduce the number of intrinsic functions that opt and code-gen has to deal with, for example to have one ld_a_f16 instead of 12.


Reducing the number of intrinsics does not change the fact that the root cause of complexity here is the fact that PTX encodes instruction parameters in instruction *names*. Even with reduced number of intrinsics that map to these instructions, someone/somewhere will have to match them to appropriate instruction variant.  1:1 mapping is relatively simple to implement with tablegen and is sufficient for its intended use of generating specific instruction variant.  
The patch does not block further optimizations. E.g. with a bit of tablegen magic it should be possible to pattern-match ld_a_f16(addrpacecast(SHARED)) and replace it with ld_a_f16_shared.

Just in case -- the naming of intrinsics is also different. Intrinsics in the patch are llvm.nvvm.*W*mma, while the intrinsics in NVVM-IR-spec use the llvm.nvvm.*H*mma. For all practical purposes they should not conflict in any way with your downstream implementation.

> It simplifies our code logic.

If NVidia would send a patch with the implementation of NVVM-IR style intrinsics, I would be glad to help reviewing and getting it into LLVM.

> Take the address space optimization for an example, when we translate a generic load to specific load, we can just change the pointer type. The rests are just copied over.

I don't think this patch prevents optimizations like these.



================
Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:3882
+                      [regty, regty, regty, regty,
+                       regty, regty, regty, regty]),
+              !if(WithStride, [llvm_ptr_ty, llvm_i32_ty], [llvm_ptr_ty]),
----------------
jlebar wrote:
> I know it's tablegen and going to be ugly no matter what we do, but could we indent this as
> 
> ```
> !if(!eq(Abc#Type,"cf16"),
>     [regty, regty, regty, regty],
>     [regty, regty, regty, regty, regty, regty, regty, regty]),
> ```
> 
> ?  Right now it looks like these arrays are parameters to `eq`, which is confusing.
Grr. I wish there was clang-format for tablegen. And python. And whatever I write. :-) Fixed.


================
Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:3888
+                #!if(WithStride,".stride","")
+                #"."#Type>;
+
----------------
jlebar wrote:
> Any reason `Space` must contain a `.` but `Type` must not contain one?
Space is optional, so for generic Space is "", otherwise it's either ".shared" or ".global".
Non-optional parameters are specified without a dot.


================
Comment at: llvm/include/llvm/IR/IntrinsicsNVVM.td:3932
+                     regty, regty, regty, regty]),
+                !if(WithStride, [llvm_i32_ty], Empty)),
+              [], // Properties must be set during instantiation.
----------------
jlebar wrote:
> Should this be indented 2 fewer spaces?
Nope. It's one of the arguments of !listconcat() and is indented as such.


https://reviews.llvm.org/D38645