Address comments

ping :)

ping :)

Address review comments.

Thanks Nicolas!

Rebase and address review feedback

In D103870#2810650, @navdeepkk wrote:

In D103870#2808376, @ThomasRaoux wrote:

In D103870#2808312, @navdeepkk wrote:

This is a great addition. We can bring in a scaling op also which scales mmaMatrix by a certain value. Maybe I can take that up.

It would be nice to be able to handle most of the element-wise ops, ideally we should re-use the std ops but it looks like this would require infrastructure changes to MLIR (https://llvm.discourse.group/t/using-gpu-type-with-standard-ops/3542/2). The best short term solution is probably to add an op taking an attribute like GPU_AllReduceOperationAttr. This is a bit hacky but that would allow us to be able to generate interesting code using the mma ops.

Okay, That would allow us to use the same op and define the semantics of the pointwise op as we like. I will be happy to take this up in the near future. Let me know what you think.

In D103870#2808312, @navdeepkk wrote:

This is a great addition. We can bring in a scaling op also which scales mmaMatrix by a certain value. Maybe I can take that up.

Address review comments

Address review comments

Patch is on top of https://reviews.llvm.org/D103868

clang-format

Add missing comment.

Address review comments

In D103099#2785041, @navdeepkk wrote:

Thanks! this is something I wasn't aware of. BTW I tested these on a Turing with CUDA10.2, and they passed, but maybe they fail on some other devices.

remove extra > and rebase.

Looks good

The test had started failing with https://github.com/llvm/llvm-project/commit/81467f500f6ad106a69088bc276024c5e1938571. I'll also enable those tests in google build bots that have Tesla T4 GPUs once this is fixed.

In D103023#2778914, @navdeepkk wrote:

In D103023#2777141, @ThomasRaoux wrote:

Note that I was also considering just removing the "DOp" altogether and have it always use "COp" but I didn't know if it was consistent with the direction you have in mind. This would be a good step in the direction of potentially removing those operands altogether.
Let me know what you think.

Hi @ThomasRaoux, It Seems like DOp is redundant. Everyone(performance-centric) would use the ops in the pattern you have in the test case. So it would be good if we could simply drop DOp and use COp instead.

Note that I was also considering just removing the "DOp" altogether and have it always use "COp" but I didn't know if it was consistent with the direction you have in mind. This would be a good step in the direction of potentially removing those operands altogether.
Let me know what you think.

In D102953#2775813, @navdeepkk wrote:

Hi @ThomasRaoux,

Great to see this, But I am not fully sure why the earlier key did not work? Isn't the underlying data used to generate the hashCode for ArrayRef<> and StirngRef<>. Here are some references from llvm/include/llvm/ADT/ArrayRef.h and llvm/lib/Support/StringRef.cpp.

template <typename T> hash_code hash_value(ArrayRef<T> S) {
  return hash_combine_range(S.begin(), S.end());
}

// Implementation of StringRef hashing.
hash_code llvm::hash_value(StringRef S) {
  return hash_combine_range(S.begin(), S.end());
}

Also, I remember before moving to tablegen, MemRefTypeStorage was using this as the KeyTy,

std::tuple<ArrayRef<int64_t>, Type, ArrayRef<AffineMap>, unsigned>

and now its,

std::tuple<::llvm::ArrayRef<int64_t>, Type, ::llvm::ArrayRef<AffineMap>, Attribute>

It seems like the earlier key would have worked. Please let me know If I am missing something.

Thanks!

In D95334#2773990, @bondhugula wrote:

I just realized that getting these test cases in will mean check-mlir will fail for all those without tensor cores on GPUs (if they are configuring with NVPTX)! Can we add an -DMLIR_ENABLE_CUDA_TENSOR_CORES and have these tests run under that? @ftynse @ThomasRaoux

LGTM

Nice!