Thank you for adding this.

Thank you!

I wanted to use presubmit bots to test this. This was now landed independently.

Just some minor nits.

Looks good conditional on small nits.

Than you @powderluv for describing your usecase. I was not aware that linalg is used as a storage/transit format.

I am in favor of landing this (but please ensure consensus before doing so). I have argued against changes before that were purely aesthetic but introduced a lot of churn (like the one in https://reviews.llvm.org/D133076). Ultimately it is a subjective decision. I have a different opinion in this case because the linalg dialect has a smaller usage scope compared to arith, so I am willing to accept more churn. Also, this change improves ergonomics when reading linalg IR, which we increasingly do and hence care about. This was also one of the motivations to introduce new operations to linalg like map.

Thanks for adding this.

Nice. Just some comments.

This looks reasonable to me but @tra or @bkramer would be better reviewers.

I agree that this goes in the right direction, however, I am unsure what this direction is? Is there a writeup of where this should be going? That would make reviewing these changes easier.

Thanks for refactoring. Having this in a separate interface addresses my concerns about the partial interface. Did not look at the deep details but also do not want to block this.

Thanks!

Thanks for doing this. The patch looks correct to me.

Not sure about the defaults but looks good otherwise. Also +1 to @bondhugula comments. Please address those first.

In D133911#3792705, @ezhulenev wrote:

gpu.memset operations expects both arguments to have the same type:

%0 = arith.constant 0 : ui32
%1 = memef:alloca: memref<ui32>
gpu.memset %1, %0 : memref<ui32>, ui32

Can you explain why this is needed? In usecases I have seen, the entire IR typically transitions from signed to signless types but I have not seen the need for a mixed form, yet.

Thanks for the explanation. My bazil-foo is not as strong as yours.

+1 for the direction. Just a bazil question.

In D133076#3764427, @Mogball wrote:

In D133076#3763423, @ftynse wrote:

I am going to go against the majority opinion here and say the syntax should just stay as it is. Most operations don't have their operands in parentheses and have their mandatory attributes as keywords. Introducing a less common syntax breaks the reading flow and unnecessarily anchors attention. The only reason why I think the current syntax may give the impression of the predicate being an operand is the comma between the predicate and the operand: arith.cmpi eq, %a, %b as opposed to arith.cmpi eq %a, %b. I also think that even rudimentary familiarity with the IR structure is sufficient to understand that the predicate is not in fact an operand: operands are values, and values are always prefixed with a % sign.

Yeah, now that you mention it, it really is just the comma. I kind of like your suggestion better.

On the other hand, the amount of churn required to essentially remove a comma is scary. For me, it's not worth it.

I understand where you're coming from ( ;) ). I may be alone in this opinion, but I'm not comfortable with the idea that we shouldn't do things just because of churn. Over a comma (and a few braces), however, I'm willing to relent.

Thanks!

I find it very strange that arith.select supports memref in the first place. Are there other operations in the arith dialect that do this?

In D131053#3697777, @mravishankar wrote:

What this does is reducing coupling between tiling linalg operations and the specific way the tiling is expressed in IR. It is true that this enables out-of-tree uses but MLIR is an infrastructure, so enabling out of tree uses should be a goal. There is no inherent reason why tiling a linalg operation should depend on using tensor operations.

I think a usage/API that is driven by a particular use of MLIR out-of-tree would become hard to maintain. I am looking for a more flushed out description of how any out-of-tree user is expected to use it (and thereby concretize the usage that is driving these changes).

In D131053#3696874, @mravishankar wrote:

This is increasing the API surface area of Tiling transformation, when in reality we need to be going the other way, reducing the API surface area.

Can you provide some rationale for this change in the description? I assume this enables reuse of the functionality independent of extract_slice and friends but making this clear in the description would be nice.

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!