I rebased, can you check the premerge to see if it does what you expect? (I'm not sure what you want to look at)

rebase

Thanks!

Yeah probably best to rename it as the same time here. This won't break anyone considering the current state!
Thanks.

(I didn't make it explicit, but the overall approach makes sense to me)

Does it mean that there are no in-tree user for this API?
Are you gonna rename this to reserve as a follow-up patch? I'm not sure I got entirely why not rename it now (that is this API is broken anyway...)?

Just some quick skimming through, I had a few comments, hope this helps! :)

Thanks for improving the doc! Are you moving this to be used in canonicalization next?
I think a first good step would be to make it a pattern and test it with a pass that applies it in the greedy rewriter. I would land this first and then try to enable this in the canonicalized.

That was the sense of my question about canonicalization indeed :)

In D124750#3502401, @srishti-pm wrote:

In D124750#3502295, @mehdi_amini wrote:

You're telling me "what" while I'm actually more interested in the "why" here?

I'm not sure what your question is, with a "why". Let me think about this a bit. I'll get back to you.

In D124750#3502228, @srishti-pm wrote:

In D124750#3500748, @mehdi_amini wrote:

Seems like this should be added to canonicalization? The "push constants to the right hand side" is there already.

I think this was not added to canonicalization because we wanted it to be an independent utility that can be used if needed and not be used if not needed.

LGTM, but please update the title and description.

Seems like this should be added to canonicalization? The "push constants to the right hand side" is there already.

We should resolve the discussion in the tracking bug first I think: https://github.com/llvm/llvm-project/issues/54242#issuecomment-1063520117

Does this print how to run the python file in case of CMake failure?

Thanks for fixing this!

they can get the same behavior by using the SameOperandsAndResultShape and SameOperandsAndResultElementType traits.

Thanks! That "expected non-function type" message annoyed me so many times :)
(I suspect it will in other contexts than the one you're fixing)

Process one action in the current thread before waiting for the tasksGroup.

Adding Chris for visibility, I know he was thinking about our threading model and utilities recently!

Add a comment on the wait()

Remove futures and use taskgroup.wait()

per-project installation directory (useful for standalone)

It may be also safe to ignore, it's not this patch that's responsible for this.

LGTM, but please wait for others!

How do you see the list declared?
I mean we can list std::function because we include the header, but would you / could you forward déclaré absl, folly, boost, etc. equivalent here? I feel I missed something?

In D124048#3468331, @cpillmayer wrote:

I don't have commit access, so it would be great if you could land the patch for me. Please use "Christoph Pillmayer cpillmayer@gmail.com" for the commit.

Sorry, I missed this... Can you please rebase? There is a conflict in a test.

We have some pieces of data though: the number of people who complain about the build filling up their disk and the link taking forever (or OOM). On the other hand the people who "know" are already setting up an explicit value...
That said I tend to think that the audience is heterogeneous enough that I agree it seems hard to make sure we "pick a default that's "right" " in the absolute (hence my proposal to not pick any).

LGTM!
Do you have commit access?

In D124153#3466424, @tstellar wrote:

I disagree about erroring out when CMAKE_BUILD_TYPE is undefined. We want to make the build easy for new users, so I think it's important that the default build (cmake with no arguments) just works.

Can you please include the regex for the update of mlir files in the description?

LGTM overall!

I don't understand the rational about turning assertions on. To me changing the default is about making it easier for users who just want to use LLVM to get the right version running, while assertions are targeting developers.

In D123915#3458332, @mehdi_amini wrote:

We need something more -- to force the printer to use the custom form at the risk of crashing. Are you planning to handle this in this revision?

No: I’ll land this as is and then see how the printer can be forced to assume the ir is verified

I don't quite get the merging yet, I need to wrap my head around it...

I patched MLIR for this:

diff --git a/mlir/test/lib/Dialect/Test/TestDialect.cpp b/mlir/test/lib/Dialect/Test/TestDialect.cpp
index 5cfefbb55971..699a365a1e2f 100644
--- a/mlir/test/lib/Dialect/Test/TestDialect.cpp
+++ b/mlir/test/lib/Dialect/Test/TestDialect.cpp
@@ -1168,6 +1168,15 @@ void StringAttrPrettyNameOp::getAsmResultNames(
         setNameFn(getResult(i), str.getValue());
 }

Slightly more elaborated:

$ ./bin/mlir-opt /tmp/test.mlir -mlir-print-operation-users
module {
  func @pretty_names(%arg0: i32, %arg1: i32) {
    // %arg0 // users: %q_2, %q
    // %arg1 // users: %q_6, %q_2
    %c0_i32 = arith.constant 0 : i32 // users: %q
    %q, %q_0, %q_1, %r = "test.custom_result_name"(%c0_i32, %c0_i32, %c0_i32, %arg0) {names = ["q", "q", "q", "r"]} : (i32, i32, i32, i32) -> (i32, i32, i32, i32) // users: %q_6, %q_2
    %q_2, %q_3, %q_4, %r_5 = "test.custom_result_name"(%q_0, %q, %arg1, %arg0) {names = ["q", "q", "q", "r"]} : (i32, i32, i32, i32) -> (i32, i32, i32, i32) // users: %q_6
    %q_6, %q_7, %q_8, %r_9 = "test.custom_result_name"(%q, %r_5, %q_2, %arg1) {names = ["q", "q", "q", "r"]} : (i32, i32, i32, i32) -> (i32, i32, i32, i32)
    return
  }
}

Actually I patched your file to try it:

module {
  func @pretty_names(%arg0: i64) {
    // %arg0 // user: %q
    %c0_i64 = arith.constant 0 : i64 // user: %q
    %q, %q_0, %q_1, %r = "test.custom_result_name"(%c0_i64, %arg0) {names = ["q", "q", "q", "r"]} : (i64, i64) -> (i32, i32, i32, i32)
    return
  }
}

Can you add tests to cover all this? I'd to see how it looks in practice!

Thanks for fixing the doc :)