In D128575#3611124, @bondhugula wrote:

Please add a test case as well for the new op.

Please add a test case as well for the new op.

@herhut - would you be able to also review this?

In D127076#3561425, @Groverkss wrote:

While adding tests, I noticed affine_map's like:

#map = affine_map<(d0, d1)[mod] -> (d0 + d1 + mod)>

wouldn't be parsed, even though they are correct according to the grammar. I added support to parse these affine_maps too.

This is a great improvement - thanks!

In D102799#3537347, @Hardcode84 wrote:

Isn't the whole point of canonicalization to convert semantically equivalent codes to the single form? So we should either always merge nested loops or always split them (which makes nested loops support in scf.parallel useless).

LGTM - should have definitely been a regular pass. Some minor comments.

LGTM - thanks. There aren't indeed any use cases to attach Values to local identifiers. Some minor comments.

LGTM

Looking mostly good. Some minor comments and a question to under the async dep related guard.

In D121279#3466770, @csigg wrote:

Here is a repro:

func @copy(%arg0: memref<1xi8>, %arg1: memref<i1>) {
  %0 = arith.constant 0 : index
  %1 = memref.view %arg0[%0][] : memref<1xi8> to memref<i1>
  gpu.memcpy  %1, %arg1 : memref<i1>, memref<i1>
  func.return
}

mlir-opt --canonicalize removes the memcpy when it really shouldn't.

Adjust error message.

Rebase.

In D123893#3458351, @mehdi_amini wrote:

The revert seemed totally warranted here: I believe the real break of policy here is that it was committed over a clearly stated objection without an attempt to resolve the discussion, this is just not OK.

In D123893#3455958, @stellaraccident wrote:

Hey folks, I'm a bit dismayed by how this patch/discussion/revert happened. It seems to me that we are reaching for perfection on a debugging option that could have added utility/mileage towards an ultimate better state. For a lot of such things, the on the ground experience of using something for debugging in varied circumstances is how we find the best outcome. I personally find merit in points that both Chris and Mehdi made for the future but I don't see a need to resolve them or get there in one step.

In any case, merits/opinions aside, this was not a shining example of either the development process or the meta goal of having an open, pragmatic community. Not sure what to do about that but I feel it needs stating. I don't have much of a stake in this issue and if Mehdi/Uday would like to discuss it, I'd be open to facilitating.

In D123915#3458296, @mehdi_amini wrote:

In D123915#3458280, @bondhugula wrote:

In D123915#3456042, @jpienaar wrote:

Nice, good point about the change to dump for invalid IR, this seems to provide the functionality of the previous change without needing a flag/API change and producing the prettier output in more cases. Of course the discussion still ongoing but looks like general improvement.

Not completely. It doesn't provide you with a custom assembly when the IR is invalid -- you *will* need an extra flag to say you need custom assembly even when the IR is invalid.

Your patch didn’t handle invalid IR, as I shown with the examples in one of my last answers there.

In D123893#3456002, @mehdi_amini wrote:

Added a test case in https://reviews.llvm.org/D123916 to double-check my understanding and illustrate my point. I added a pass that creates an invalid op and has an option to trigger an optional call to "signalPassFailure()".
Below is the trace for all the possible configurations (the only constant is that the IR is invalid) with the current patch applied:

• The pass signals pass failure, the option -mlir-print-custom-assembly-after-failure is not set:

$ bin/mlir-opt ../mlir/test/Pass/invalid-ir.mlir -pass-pipeline='func.func(test-pass-create-invalid-ir{signal-pass-failure=true})' -mlir-print-ir-after-failure 
// -----// IR Dump After (anonymous namespace)::TestInvalidIRPass Failed //----- //
"func.func"() ({
  "test.any_attr_of_i32_str"() : () -> ()
  "func.return"() : () -> ()
}) {function_type = () -> (), sym_name = "TestCreateInvalidCallInPass"} : () -> ()

• The pass does not signal pass failure (so the verifier will fail instead), the option -mlir-print-custom-assembly-after-failure is not set:

$ bin/mlir-opt ../mlir/test/Pass/invalid-ir.mlir -pass-pipeline='func.func(test-pass-create-invalid-ir{signal-pass-failure=false})' -mlir-print-ir-after-failure 
<unknown>:0: error: 'test.any_attr_of_i32_str' op requires attribute 'attr'
<unknown>:0: note: see current operation: "test.any_attr_of_i32_str"() : () -> ()
// -----// IR Dump After (anonymous namespace)::TestInvalidIRPass Failed //----- //
"func.func"() ({
  "test.any_attr_of_i32_str"() : () -> ()
  "func.return"() : () -> ()
}) {function_type = () -> (), sym_name = "TestCreateInvalidCallInPass"} : () -> ()

• The pass signals pass failure, the option -mlir-print-custom-assembly-after-failure is set:

$ bin/mlir-opt ../mlir/test/Pass/invalid-ir.mlir -pass-pipeline='func.func(test-pass-create-invalid-ir{signal-pass-failure=true})' -mlir-print-ir-after-failure  -mlir-print-custom-assembly-after-failure
// -----// IR Dump After (anonymous namespace)::TestInvalidIRPass Failed //----- //
"func.func"() ({
  "test.any_attr_of_i32_str"() : () -> ()
  "func.return"() : () -> ()
}) {function_type = () -> (), sym_name = "TestCreateInvalidCallInPass"} : () -> ()

• The pass does not signal pass failure (so the verifier will fail instead), the option -mlir-print-custom-assembly-after-failure is set:

$ bin/mlir-opt ../mlir/test/Pass/invalid-ir.mlir -pass-pipeline='func.func(test-pass-create-invalid-ir{signal-pass-failure=false})' -mlir-print-ir-after-failure -mlir-print-custom-assembly-after-failure
<unknown>:0: error: 'test.any_attr_of_i32_str' op requires attribute 'attr'
<unknown>:0: note: see current operation: "test.any_attr_of_i32_str"() : () -> ()
// -----// IR Dump After (anonymous namespace)::TestInvalidIRPass Failed //----- //
"func.func"() ({
  "test.any_attr_of_i32_str"() : () -> ()
  "func.return"() : () -> ()
}) {function_type = () -> (), sym_name = "TestCreateInvalidCallInPass"} : () -> ()

We print generically in all cases: the current patch has no effect when there is invalid IR involved.

Any more comments here @csigg ?

In D123915#3456084, @mehdi_amini wrote:

In D123915#3456079, @lattner wrote:

Why is this safe? Does the printer already check that the IR passes verify() and falls back to the generic form automatically?

If so, this is awesome. Does Operation::dump() have the same behavior?

Yes: this changed ~ last month I think.

That is what I was trying to explain in Uday's patch and why I was objecting to adding a flag.

In D123915#3456042, @jpienaar wrote:

Nice, good point about the change to dump for invalid IR, this seems to provide the functionality of the previous change without needing a flag/API change and producing the prettier output in more cases. Of course the discussion still ongoing but looks like general improvement.

Any other comments here: @rriddle?

@ftynse @herhut -- any other comments here? Everything's addressed.

In D123893#3455771, @mehdi_amini wrote:

In D123893#3455768, @mehdi_amini wrote:

In D123893#3455709, @lattner wrote:

What is the concern here? I don't think that Uday is arguing this should be on-by-default.

Uday doesn't: I am.

To expand, my proposal is:

1. Change the default for a pass failure (which what this patch is about) to print the custom form, that is remove the printGenericOpForm() from mlir/lib/Pass/IRPrinting.cpp) . This has nothing to do with invalid IR: printing the custom form will (by default) run the verifier and on failure fallback anyway to generic IR already right now.

Update cmd line flag name and added test case.

In D123893#3455615, @mehdi_amini wrote:

In D123893#3455396, @jpienaar wrote:

The cost of getting custom format output from generic output dump is rerunning through opt tool, the cost of crash is rerunning program (which can take multiple hours and take a lot of compute resources). I'd much rather it be uniform and safe and avoid needing flag plumbed through.

I have another take on this: this is printed after a pass failed but we don't know that the IR is invalid.
We were printing generically to be conservative, however our printer is now "safe" in that it'll run the verifier and print generically anyway (even with this new option added here...) if the verifier is failing.

In D123893#3455396, @jpienaar wrote:

The cost of getting custom format output from generic output dump is rerunning through opt tool,

Title: spec -> doc

Fix typos.

In D123266#3448690, @ThomasRaoux wrote:

In D123266#3448574, @herhut wrote:

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

Please also wait for @bondhugula, who had concerns.

Thanks @herhut. @bondhugula, do you still have any concerns?

Thanks for the review, @ftynse, @herhut. Fixed and enhanced the check. Tested via SCCP as well.

In D123756#3450844, @rriddle wrote:

Wouldn't have expected this given the static assert, but weird.

Switch to {}.

Add a couple more test cases for the outlining pass.

Missed updates for gpu.launch -> gpu.launch_func.

In D123499#3447805, @bondhugula wrote:

In D123499#3445734, @csigg wrote:

For the support in this PR, I'm not adding a result token to the op if no async deps have been specified (even if the async keyword is specified, it's dropped during the print). It should perhaps be a parse error and similarly for the launch_func op?

I don't think this is a good idea. The 'async' keyword stands for the !gpu.async.token return type and should really be independent of the token operands inside []. I would not infer the former from the latter, but fail verification (for all but gpu.wait) if we want that.

This makes sense to me too -- follows from the previous para. I'll update the revision to make sure there is a meaning to the "async" keyword (regardless of the tokens) in that it returns a token indicating async execution.

Fix semantics and syntax to allow async without any deps.

This is looking good to me. Thank you for contributing these. Mostly minor comments/requests for documentation.

In D123499#3447326, @mehdi_amini wrote:

In D123499#3445734, @csigg wrote:

For the support in this PR, I'm not adding a result token to the op if no async deps have been specified (even if the async keyword is specified, it's dropped during the print).

I don't understand what you're describing: you don't get to control the number of output of the operation, this is parsed by MLIR before your custom parser and if you don't provide a result type matching the number of result MLIR will fatal_error() anyway.

In D123499#3445734, @csigg wrote:

async with 0 async dep tokens wouldn't appear to be a meaningful configuration for the op. The lowering does check for it and fails, but should it just be disallowed?

I'm not sure. I would say the semantics are clear if an op uses async (the host does not wait for the op to complete) but no dependencies (it can run immediately without waiting for anything else), and it's OK for the current lowering to be limited in what it can handle and rely on gpu-async-region to bring it into lowering-compatible form. I kind of like the symmetry of these ops (including gpu.wait, where gpu.wait async [] needs to be valid).

In D123482#3444729, @bondhugula wrote:

In D123482#3444626, @mehdi_amini wrote:

You broke the bot apparently: https://lab.llvm.org/buildbot/#/builders/61/builds/24891 ; can you look into this?

/vol/worker/mlir-nvidia/mlir-nvidia/llvm.obj/bin/mlir-cpu-runner: symbol lookup error: /vol/worker/mlir-nvidia/mlir-nvidia/llvm.obj/lib/libmlir_cuda_runtime.so: undefined symbol: cuMemAllocAsync

The bot has an old version of CUDA that doesn't support async alloc. We already guarded it with >= 11.2 above, and I double-checked again that a 11.2 or higher version should have had that method. Looking at the logs, the bot has CUDA_VERSION=10.2.89 in its env (not sure what the preprocessor sees). Either the format is different which is tripping the macro check or the build is being compiled with newer headers but is being linked with older libraries. Someone with access to the bot will have to see what CUDA_VERSION is being set to (for the build preprocessor).

In D123482#3444626, @mehdi_amini wrote:

You broke the bot apparently: https://lab.llvm.org/buildbot/#/builders/61/builds/24891 ; can you look into this?

Base it on the right commit.

Rebase.

Sorted order.

Add AsyncOpInterface to gpu.launch and move common methods in GPUDialect.cpp up.

In D123499#3442878, @csigg wrote:

A gpu.launch_func without async implies that it is synchronous (same for gpu.memset, gpu.memcpy etc). If the lowering to the gpu runtime wouldn't reject it, it would need to insert a mgpuStreamSynchronize call.

In D123499#3442565, @csigg wrote:

Not opposed to this change at all, but what's the motivation for allowing gpu-async-region to run before gpu-kernel-outlining?

Update commit summary.