Cool, great start!

Some more comments addressed.

The answer is simple. I did not rebase :)

Commit message fixed.

Noticed this because the loop.parallel transformation bailed out on these.

https://reviews.llvm.org/D74480 for the mlir-cuda-runner changes. I had to include many dependencies that I assume should come with the JitRunner. Not sure what the overall goal is but this makes things build.

PTAL

include files for the win :-)

Feel free to remove the changes to log. But with your change it seems even less consistent than before.

In D73893#1865719, @mravishankar wrote:

(Sorry for jumping on this late)

This is now adding three different lowering of loops to GPU

1. From perfectly nested loops to GPU
2. From imperfectly nested loops to GPU
3. Parallel loops to GPU I confess I contributed to one of them (2). We should probably have a plan to consolidate these. This is too much duplication of functionality. We should probably deprecate (2) in favor of this change, and adapt all uses accordingly.

Comments, comments, comments :-)

Some small comments. With changes syntax this looks great.

Drop llvm.

In D74389#1868977, @ftynse wrote:

This would work for me. At scale, we may need to analyze effects of such approach on compile time. The converter essentially twice the number of patterns it has to search through, and there are potential rollbacks of rewrites that generated illegal operations.

In general, I think we want to expose individual LLVM patterns anyway because several people hit the selection problem repeatedly. I haven't had time to do that yet.

Writing this in code for discussion. Is this the way to resolve conflicts or do we want to hand-pick patterns from the LLVM conversion? The latter seems brittle as well, as the number of pattern keeps growing and we need to track. By disallowing, we can do this whenever we add a new intrinsic.

Explicit lambda instead of std::bind.

Thanks! This looks great. I had written some comments but forgot to hit the submit button it seems :(

Thanks for starting this. I'd be happy to see this land.

Thank you Christian for adding this!

Correct tests and some minor cleanup.

Also support linalg tiled loops

In D74012#1859486, @antiagainst wrote:

In D74012#1858709, @mravishankar wrote:

I understand what the intent is here, but the input already has an attribute that belongs to the SPIR-V dialect before lowering.

Generally I think it is inevitable that we'll have attributes belonging to lower layers attached at the source input at a higher layer, for example we will have spv.target_env and spv.interface_var_abi attributes attached to dispatch regions isolated by IREE at HLO level. The root problem is that we cannot infer everything a lower dialect needs all from higher dialects and it does not make sense to create duplicates all the way up to every layer. But with that said,

That makes things a bit non-composable. In cases where someone lowers to the GPU dialect and then conditionally decides to lower to SPIR-V dialect or the NVVM dialect, with this change on the SPIR-V side a separate pass will be needed to add this attribute. Ideally the input should be only in GPU dialect, whereas here it isnt.

Here we are at the boundary between GPU dialect and SPIR-V dialect; so it should be fine to have SPIR-V specific stuff attached to the input to drive conversions towards SPIR-V. But I get your idea here regarding non-composability. If the input is, say, loops, it's better to have a proper GPU dialect attribute instead of spv.entry_point_abi attached to loops for driving further conversion.

Is it possible instead to add an attribute to GPU dialect itself which contains information about the workgroup size. Then while lowering we can convert one attribute to another.

Yeah that makes sense to me. At GPU level we also have such concepts so we can have similar attributes; they are just contracts to different layers. Right now we are using a bunch of command-line options for doing that job; I'd love to see us switching to use attributes there too. I've created https://llvm.discourse.group/t/using-attributes-to-specify-workgroup-configuration-when-lowering-to-gpu/496 to RFC. I view that as an upper layer above SPIR-V so it's a bit separated from the changes here IMHO.

In D74041#1859977, @mehdi_amini wrote:

In D74041#1859889, @herhut wrote:

I am setting up a build bot that will build the tree with the cuda parts enabled so we can see this easier.

I updated our build kite bot accordingly already: https://buildkite.com/mlir/mlir-core

I am setting up a build bot that will build the tree with the cuda parts enabled so we can see this easier.

Great to see progress on this! Some comments.

https://reviews.llvm.org/D74041 should fix the link errors.

use is_assignable

There is probably a prettier way to do this but this unblocks building.

In D73713#1858103, @mehdi_amini wrote:

In D73713#1856579, @herhut wrote:

Let's use dynamicallyIllegalOp instead to disallow the CPU intrinsic explicitly. That would also make the non-determinism go away by not relying on pattern order anymore. And it also captures the actual issue that the intrinsics call is indeed illegal.

IIUC: This does not address the underlying issue with the framework though, it seems to still be in the category of "hiding the bug".

Formatting.

@rriddle Is this what you had in mind?

Less templates.

Remove attribute from td file and minor cleanup.

In D73893#1855737, @ftynse wrote:

I suppose you want high-level feedback on this, so I'm not nitpicking in the code.

Remove unneeded import.

Made it a template with help from @pifon2a.

In D73893#1854949, @nicolasvasilache wrote:

2. one of the problems you solve here is to go from type (the affine maps for mapping) to values (tidx, bidy, etc)). This is a recurrent transition that would be great to try and make retargetable (e.g. OpenMP or other runtimes), I would love to see this separated from GPU. This would also force separating concerns a bit more re outlining of kernel vs mapping of loops.

Let's use dynamicallyIllegalOp instead to disallow the CPU intrinsic explicitly. That would also make the non-determinism go away by not relying on pattern order anymore. And it also captures the actual issue that the intrinsics call is indeed illegal.

This seems good to me but the tests are failing. From just looking at the code not sure why.

Maybe this should even be a template to cover more cases like BlockArguments.

I started implementing the lowering of parallel loops to gpu launch code. This code uses mapping annotations in the form of attributes. Currently, one can only map to a single block/thread id. Also, upper bounds for the launch can be defined based on the number of iterations of the mapped iteration of the parallel loop. This is probably not what we want in the long run but a good starting point to inform the design we take.

Looks good to me. I wonder about users of TiledLinAlgOp. Are there none? This should be a breaking change.

Review comments.

In D73620#1850048, @mehdi_amini wrote:

Why was this committed while we're discussing here?

Whoohoo. \0/ Thanks!

I went for lifting the requirement that gpu.launch needs to be closed from above. So we no longer have any arguments.

Thanks!

PTAL. I will do another breaking change that removes the use of region arguments in a followup.

Split out gpu dialect cleanup parts.

In D73465#1843770, @mehdi_amini wrote:

Code motion into a gpu::LaunchOp region requires operands of moved instructions to be threaded through operands of the gpu.launch to ensure that the gpu.launch remaines closed from above.

Can we revisit lifting this requirement? This was something that was raised from the beginning of the development of this op.

Nice catch!

Thank you for adding this!

Review comments...

Thanks.

I have reviewed this before (the basic approach has not changed). Thanks for adding more comments and tests.

I browsed over the code and left some minor comments. The idea and code in verifiers LGTM. I find it much harder to read now with all those similar sounding accessors but that is inevitable I assume if you want to support mixed forms.