Nice, thanks! Fine to land as a stretch improvement but I think we need to further sugar the new exception class to avoid exposing implementation details.

Thanks. I had started reviewing this but got side tracked with the archaeology. Thanks for simplifying -- this was always not great.

Python changes lgtm. The parser changed make sense to but I usually defer those reviews to river.

Thanks!

In D141804#4117244, @nicolasvasilache wrote:

In D141804#4117123, @mehdi_amini wrote:

In D141804#4066909, @powderluv wrote:

In early December we switched back to textual representation (at the cost of storage space) to provide better stability of the IR.

How is this supposed to be more stable? What were you using before?
The bytecode is intended to be more stable than textual IR (even though not resilient to Dialects changes at the moment, pending some versioning work)

I think this may be a case of socialization / familiarity with that part of the system.
I haven't personally looked at how to use the bytecode, I see there is doc, should we have something in the Tutorial ?
Serializing string always seems tantalizingly easy until the castle build on sand starts to crumble..

In D141804#4060009, @mravishankar wrote:

To start with, I see at minima design consistency with the general dialect design I think I saw upstream so far.

Linalg ops syntax is

%foo = linalg.<some-op> ins(... : <list of input types>) outs(... : <list of output types>) -> <return types>

Ops AFAIK in general are

%foo = <some op> : <inputtypes> -> <result type>

for the most part.... So linalg ops are anyway not consistent. If we want to evolve Linalg ops syntax, then can we do it in one shot to a state that has general consensus. Also in terms of consistency, these are only for named Linalg ops and not linalg.generic, which by itself is not consistent.

Makes sense, let's work towards this! Shall we start a shared doc and go through IR samples, including various dialects, and try to put together a "style guide for dialect syntax"?

@mehdi_amini that would be fantastic!
This needs a serious owner to really lift these ops from their "assembly-level" status and I have not been able to do that part.
One thing I have always been frustrated about is the syntax delta between a linalg.generic and a simple math expression.
@stellaraccident and @gysit provided huge improvements to make it better at the python level but the IR remains very much underserved.

The ability to "see" computations as e.g. %C(i,j) = arith.addf(arith.mulf(%A(i, k), %B(k, j)), %C(i, j) brings a phase shift in cognitive capacity.
See "Fig. 6. TC Benchmarks used in the experiments." in https://dl.acm.org/doi/pdf/10.1145/3355606.
It was e.g. very useful to understand the anti-diagonal reduction pattern on the LHS operands in the kronecker3 example.

Given the security situation, I am fine making this change to tighten things up. It may cause some trouble for folks in the wild if they have concerning versions and pip doesn't offer great solutions. As part of a dev setup, though, I feel that folks should be able to self serve solutions to issues of there is a problem.

In D140088#4015926, @nikic wrote:

Is there an RFC for this change? In https://discourse.llvm.org/t/rfc-add-apfloat-and-mlir-type-support-for-fp8-e5m2/65279 the addition of an LLVM IR type was explicitly out of scope. See also in particular the comment by @efriedma here: https://discourse.llvm.org/t/rfc-add-apfloat-and-mlir-type-support-for-fp8-e5m2/65279/3

I'm personally rather skeptical about adding new ad-hoc floating-point types to IR without at least some restructuring of the existing system. At the IR level, my expectation would be that all the different float types get consolidated into a single type that is parameterized over the floating-point semantics.

Perfect. Thanks for checking. Do you need help landing this patch?

Thank you! Do you need help landing this patch or do you have commit access?

Thank you Jacques/River -- I found this one in the field when only running release binaries on windows. It was not fun to get the repro that triggered it. Really glad we're continuing to improve reprocucers as they are a life saver.

Hi Renato - I've reviewed and this lgtm, but I rarely work on/review this part of the codebase. Do you feel like you are a good reviewer for it (the author reached out to me asking for some help coordinating reviewers)?

Lgtm modulo trivial comments

Thanks. Do you have commit access? Or do you need help landing?

In D134812#3955348, @phawkins wrote:

Gentle ping: can this be merged? The same bug got reported again.

(change lgtm and I think we may want to rework this API at some point)

Thanks for having a look. I left a note on the issue about possibly doing this differently. WDYT? https://github.com/llvm/llvm-project/issues/59212#issuecomment-1328315420

As mentioned, the duplication is annoying but I suspect is the least bad way to do it.

Thanks - this is great!

Thanks for this. Patch lgtm. Has a couple of format issues but probably ok as-is (this file is very inconsistently formatted and it looks like you overrode clang-format a bit for consistency with adjacent code).

In D136404#3885547, @ftynse wrote:

I don't have a strong opinion. We don't (yet) offer any sort of API stability guarantees at this level, so just changing it is fine. There will be a compiler error, it's not like changing the type from one pointer to another. On the other hand, having mlirPassManagerCreate to work on builtin.module sounds like a reasonable default and we can have a more verbose mlirPassManagerCreateOnOperation that also needs a more verbose argument.

Sorry for the delay - I attempted to rebase/land but the patch conflicts. I don't have time to actually update it right now - can you rebase/fix? I'll be back from traveling later in the week and can do the rebase/fix if you don't get to it but am not in a position to be making actual edits right now.

Another contributor cloned and submitted this with suggested fixes as a new patch: https://reviews.llvm.org/D135650

Ran the full clang/llvm/mlir test suite in debug mode just to be safe.

Remove break after llvm_unreachable for consistency with other switches in file.

Switch to explicit case per comment from ctopper.

Add fix to MicrosoftMangle.cpp that caused buildbot failure.

I'm personally ok with this patch. If we decide to go forward with the index dialect, it will come with a transition plan for index-in-arith, and it doesn't seem like this makes that harder. If anything, it makes it easier to specify what the transition path is for the existing index cast ops.

Rebase.

Go ahead and land at your convenience. I'll land the other one once you do (busy and not going to get to it today)

Do you need help committing? The other one does - so if you both do, I can do final fix-ups as I'm landing both.

Also note https://reviews.llvm.org/D134812 in flight. I think that one should go in first

Also note https://reviews.llvm.org/D133450 in flight. I think yours should go in first

Good catch - thanks.

Bkramer; was your intention to review both the APFloat and MLIR side of the patch? Just making sure that the MLIR side had a proper look before submitting and it wasn't clear to me from the commentary.

In D133823#3815492, @rengolin wrote:

Honestly, there isn't much meat here, other than the decision to support and the name bikeshedding, both of which I think are well covered in the RFC.

In D133823#3813169, @bkramer wrote:

Code looks good. Were the concerns about naming resolved?

Formatted and naming suggestions from RFC applied. Ready for review.

git-clang-format

Format and apply rename suggestions.

Oh, wow - not sure how that was missed. Thanks.

In D133823#3789844, @rriddle wrote:

I imagine APFloat changes likely require an RFC (though I'm not sure). Also, did you accidentally reformat all of the APFloat tests (4k lines change o.O)

In D133076#3767827, @jsetoain wrote:

Further, for these matters of style/taste, I think that barring a rationale (which can be consistency based like you point out) or strong opinions that one form is better, we should bias towards what is. That is how mature projects operate, ime, and the decision making process working something like this is kind what I'm looking for.

We don't have any of this written down, and maybe we should. Thus my comment about a style guide -- the traditional way to codify both specifics and rationale for such things.

My two cents, for what is worth. 100% in favour of some sort of style guide. It already came up a couple of times in the past for me. I find consistency in syntax very valuable, it makes dialects predictable and easier to understand. In an ideal world, whenever I see an op in a dialect I haven't worked with before, I should have a decent idea of what it does. You can see some syntax convergence, but also exceptions. It's annoying. I'm not sure if it's too late to create something sensible at this point, or even if it's possible in something of MLIR's nature, but I think it's worth considering. Naming conventions for ops, types, and attributes, operand order, keywords for different types of operations (like the into and from in data movement operations).

I offer myself as volunteer to start some sort of draft collecting common practices in the core dialects, if it's any help. It doesn't even need to be prescriptive, it might be valuable as a reference for people starting a new dialect 🤷Thoughts?

In D133076#3767430, @Mogball wrote:

(sarcastic) So we should stage the removal of the comma by first making it optional, parsed with a warning that "The comma is deprecated and will be removed on Sept 10" to give users time to migrate.

In all seriousness, it was useful gauging the feedback for a relatively minimal change with big user impact. My concern specifically is that if we are in the position of introducing new compare ops (e.g. in new dialects), "keeping it consistent with arith" means adopting a perhaps less-than-optimal syntax. Hence, arith should be updated to a "better" syntax, which other dialects can copy.

In D133076#3765466, @stellaraccident wrote:

Personally, I'm +1 on cleaning things up but -1 that this is an actual improvement in consistency and legibility (based on a cursory review of the prevailing style). I'm with ftynse's interpretation, and while I could see the argument for removing the comma, I don't think that is an improvement either.

Personally, I'm +1 on cleaning things up but -1 that this is an actual improvement in consistency and legibility (based on a cursory review of the prevailing style). I'm with ftynse's interpretation, and while I could see the argument for removing the comma, I don't think that is an improvement either.

This looks good to me but I'd like to see the reviews of the other contributors to the conversation before landing.

Thanks and sorry for missing this. Ditto to what Mehdi said about pinging for things that have direct impacts like this.

I'm ok/supportive of having the option in the test pass to run with canonicalizations mixed in: it isn't a bad idea to be and to test that they work together. But yeah, I'm not thrilled about coupling that in the actual populate method.

I did add the canonicalization patterns to a downstream and have observed that they do "help" a lot of real cases I was seeing get to a fully decomposed state.

Makes sense to me in theory. Is there a kind of case we can test for that needs this?

FYI - Some intervening changes rendered this patch at least partially invalid and I never parsed back through and redid it. Consider it for inspirational value if anyone wants to pick it up.

Rebase

Thank you!

Good catch, and thank you!

Do you have commit access?

Sorry, yes - I submitted a follow-up and neglected to note on this thread: https://reviews.llvm.org/rGbeebffa9ab81bba3de91b2cee7a62578ebe1ae00

Rebase, fix standalone example, attempt to patch bazel as best I could.

Closed by https://reviews.llvm.org/rG1d6a90418e4bfc294b4174880e93cb43835ebdf5 (messed up the arcanist command and association was lost)

Note that this is associated with https://reviews.llvm.org/D129604 (messed up the arcanist command and association was lost)