In D129036#3627632, @csigg wrote:

As a follow-up, how about adding an attribute next to gpu.kernel that encodes the launch bounds?
They can be used to provide ranges to gpu.block_id etc and can be lowered to e.g.:
https://docs.nvidia.com/cuda/parallel-thread-execution/index.html#performance-tuning-directives

I don't think there are any guarantees that MLIR preserves unknown attributes during canonicalization. So this would be the first precedent in that direction.

Separate pass works for me.

If it useful beyond testing, I don't see why it should not be a regular pass.

What was the motivation behind this canonicalization? In particular, why would a combined parallel loop be considered the canonical form?

Thanks!

Thanks. Adding @csigg as an FYI.

What is the longer term plan here? Do all places that introduce alloca have to perform this local optimization? Should we instead have a pass that hoists alloca out of loops? Or rethink the introduction of allocation scope for loops?

Thanks!

Looks good to me. We really need to figure out a way to group dialects :)

Thanks for adding this.

Found the testing. I need to get my docker setup in order before I can test, so this will take a little to land.

Thanks!

Is there a way to test these configurations before submitting them?

Thanks!

In D119730#3328139, @mravishankar wrote:

I have always felt this method to be a bit ad-hoc. I wanted to replace this with the logic in this patch https://reviews.llvm.org/D119904 (at line 792 in LinalgOps.cpp). That logic is better defined and easier to follow I think. If you could adapt this to use that approach, that'd be great.

Thanks!

Consider also adding an integration test. I think we have one for the other cases.

Thanks for splitting this out.

Thanks for your reference to the scoping operation @ftynse. I had the feeling we had something like it but could not find it.

My concern with only doing the scf.parallel operation is that we do lower it to scf.for loops in some cases and we would have to model this behavior there somehow. How would we do this? Do we have a suitable high-level abstraction that allows to insert an allocation scope? Alternatively, we could mark the scf.for loop the same. That seems a reasonable choice but scf.for supports returning values, so we might actually have escaping stack allocations currently. Is this a model we want to support?

Thank you. Just a naming nit.

In D117427#3254758, @akshaybaviskar wrote:

In D117427#3250053, @akshaybaviskar wrote:

In D117427#3248075, @herhut wrote:

Thanks for adding this. Could this rather reuse the logic from verifyAllocLike from the memref operations? It is the same logic but it would need to be exposed to other dialect.

Thanks for the suggestion. Is it allowed to expose this function to other dialect?

Hello @herhut,

Could you please reply to this?

Thanks,
Akshay

Thank you. The default is fairly arbitrary (well, it's good enough for some index computations) and we really should have some cost model here. +1 to making it configurable at least.

Please fix the commit message though. I just noticed this now.

Thanks.

PTAL

Pull size checking out of helper.

Thanks for adding this. I have wanted this a couple of times, too, but never went as far as creating a diff for it.

Thanks for adding this. Could this rather reuse the logic from verifyAllocLike from the memref operations? It is the same logic but it would need to be exposed to other dialect.

fix test

The arith::minf operation is defined as

Thanks!

In D116600#3234038, @nicolasvasilache wrote:

I think Mahesh's suggestion is better because there is no guarantee in shape reification that you'd return 1 and you may well end up with propagating dynamic information further down.

There could well be a separate pass that inserts these casts to avoid the behavior in shape reification if that is critical to your use cases. I think it would still be better to fix shape reification to return a static value where possible. After all, if this is valid linalg IR, reification should do as good as it can with it.

rebase

Thanks!

In D116889#3230744, @ftynse wrote:

Is it legal though? Should we have something like gpu.thread_id outside of a GPU function/module? I would rather think it is legal to inline GPU functions into GPU functions.

Thanks!

In D116099#3225198, @bondhugula wrote:

You are just using isIdentity() -- the API doesn't name map outside. In the inside, it's still a map.

I was not clear. I was wondering about the case where the static type of the memref does not have a map but the descriptor at runtime still does not have identity strides. Currently, one can create this setting using memref.reinterpret_cast by omitting the map from the target type. Maybe that should be illegal.

This would be an invalid op and should have failed the verifier in the first place. (Note that the memref always has a map -- it's not printed if it's identity as you know. Also, identity strides (all ones) don't correspond to an identity map -- N^2, N, 1 for example would correspond to an identity map for a 3-d memref for example.)

Combined the two patterns.

Simplified as suggested

Great find. Thanks for the fix.

In D116099#3206683, @bondhugula wrote:

In D116099#3206432, @herhut wrote:

In D116099#3205178, @bondhugula wrote:

Do you want to instead create one pattern with two functions in it? It'll lead to less overhead in the greedy rewrite driver and perhaps also easier to later choose between the two.

I though about that, too. Another way would be to give the memcpy based pattern higher benefit and actually make then non-exclusive. That way one could optionally blend in the memcpy based pattern.

All of this still leads to two patterns.

Can you change the subject to have an additional tag, e.g., [mlir][arith]. That provides a little more context. You could also state that it did fail for vectors before but now it folds them correctly.

Thanks!

In D116099#3205178, @bondhugula wrote:

Do you want to instead create one pattern with two functions in it? It'll lead to less overhead in the greedy rewrite driver and perhaps also easier to later choose between the two.

Add comments and failure reason.

Thanks. I don't see any reason to disallow this either and was surprised by the error message.

Add tests and fix behaviour for zero case.

It would be nice if we could just pass the data layout as a string.

I assume that would be @nicolasvasilache as I suspect this is a matmul bufferization issue.

Thanks @mehdi_amini ! That is much easier.

Use in-test tag as suggested.

This was recently fixed in https://reviews.llvm.org/D114391. If you rebase, you should pick up that change.

Please sort the headers, otherwise looks good. Thanks!