This is an archive of the discontinued LLVM Phabricator instance.

[mlir][linalg] Omit printing result types for named ops.
Needs RevisionPublic

Authored by pifon2a on Jan 15 2023, 2:57 PM.

Details

Summary

Result types for the named ops can be deduced from the list of inits/outs.

This affects only named ops generated from the YAML file, i.e. linalg.matmul, linalg.fill and the likes. After this change all Linalg ops, except for linalg.generic, will have the following syntax:

opname ins(list_of_input_args: list_of_input_types) outs(list_of_init_args: list_of_init_types).

The regex that was used to fix most of the tests: s/ outs(\(.*\)) -> .*$/ outs(\1)/g.

Diff Detail

Event Timeline

pifon2a created this revision.Jan 15 2023, 2:57 PM
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
pifon2a requested review of this revision.Jan 15 2023, 2:57 PM
mravishankar requested changes to this revision.Jan 15 2023, 3:52 PM

This is an unnecessary change that just causes downstream churn. Not sure it is worth it

This revision now requires changes to proceed.Jan 15 2023, 3:52 PM

This is an unnecessary change that just causes downstream churn. Not sure it is worth it

It makes IR shorter and nicer. It is not a lot of churn, the named ops are not as widely used as linalg.generic. 90% of the tests were fixed by a simple sed command with s/ outs(\(.*\)) -> .*$/ outs(\1)/g.

The "churn" aspect is subjective. There were patches like https://github.com/llvm/llvm-project/commit/abc362a1077b9cb4186e3e53a616589c7fed4387, that were not blocked.

This is an unnecessary change that just causes downstream churn. Not sure it is worth it

It makes IR shorter and nicer. It is not a lot of churn, the named ops are not as widely used as linalg.generic. 90% of the tests were fixed by a simple sed command with s/ outs(\(.*\)) -> .*$/ outs(\1)/g.

The "churn" aspect is subjective. There were patches like https://github.com/llvm/llvm-project/commit/abc362a1077b9cb4186e3e53a616589c7fed4387, that were not blocked.

Some other similar patches were blocked (I dont have the link handy). Apart from ergonomics, does it fix anything else. If so, I'd rather ignore ergonomics aspect of this and drop this patch.

I think this is worth cleaning this up. Small things like these can make it look like linalg is a second-class dialect where we don't care about code quality.

I think this is worth cleaning this up. Small things like these can make it look like linalg is a second-class dialect where we don't care about code quality.

Disagree with this characterization. I have explicitly heard from other downstream users (so not IREE/Google) that thing they would prefer is stability in IR format and API. Breaking changes is what causes people headaches. Also the explicit return type specification in IR makes it more explicit that a value of that type is being created by this operation. The implicit tying is more confusing to someone new to MLIR IMO.

herhut accepted this revision.Jan 16 2023, 1:26 PM

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.

The cost for these kind of changes will only grow, so I feel like we should use our chance to clean things up while we can.

Please change the description of the PR to clarify that it affects the linalg dialect and add instructions how to fix tests to the PR description. That way people do not have to read the review to know what needs to be done.

pifon2a retitled this revision from [mlir] Omit printing result types for named ops. to [mlir][linalg] Omit printing result types for named ops..Jan 16 2023, 2:29 PM
pifon2a edited the summary of this revision. (Show Details)
pifon2a edited the summary of this revision. (Show Details)Jan 16 2023, 2:29 PM
mravishankar added a comment.EditedJan 16 2023, 5:37 PM

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.

I dont see any reason why linalg ops need to be readable. Also readable means different things to different people. To me this change actually makes it less readable. Before it is clear what the result type is. With the change, you have to know the implicit type coupling between outs operands and results to understand.

W.R.T ops like map, reduce, etc. I am not sure the cost of all the named op definitions, parser/printer (and all subsequent changes that made changes to parser/printer of those ops) are really worth it. I would still highly prefer those ops be removed.

The cost for these kind of changes will only grow, so I feel like we should use our chance to clean things up while we can.

Please change the description of the PR to clarify that it affects the linalg dialect and add instructions how to fix tests to the PR description. That way people do not have to read the review to know what needs to be done.

As a meta-point: I'm concerned about the push-back on improving things based on churn. The comment about users wanting stability can only be an argument about spending more time on design and being more thorough before adding anything to the codebase. However that means much less velocity and higher cost to experiment. So far this isn't the tradeoff we've been making in MLIR I believe.

I dont see any reason why linalg ops need to be readable.

Can you elaborate? I'm surprised because taken at face value, this comment is quite against the concept of custom printer in MLIR themselves!

Also readable means different things to different people. To me this change actually makes it less readable. Before it is clear what the result type is. With the change, you have to know the implicit type coupling between outs operands and results to understand.

Eliding return types when they are coupled is quite the norm I believe in upstream dialects, what makes it different in your view here?

Why is linalg.generic different? If this is going to changed later it would be nice to batch those changes together since breaking compatibility downstream has a cost (as discussed)

@ThomasRaoux I would really like to do that, but linalg.generic allows the mixed case, when you have memrefs and tensors at the same time and some folks are using it for some reason.

@ThomasRaoux I would really like to do that, but linalg.generic allows the mixed case, when you have memrefs and tensors at the same time and some folks are using it for some reason.

Interesting, and in this case we cannot infer the result type based on operands? Isn’t the result type always matching the outs tensors?

I think Mehdi has a good point about spending more time on design. I agree clean ups are nice, however I feel like the incremental breaking changes are causing the problem and we shouldn’t take those lightly. I believe there has been several syntax changes proposed to linalg ops in the recent past? Should we make sure we have a syntax that won’t need more change in the near future before make those changes? (Maybe it is worth a quick chat on discourse to make sure we address all the potential syntax problems at once and not continuously break users). What do you think?

From my experience integrating MLIR into Google's internal repository, breakages from changes like these seem to be very frequent (e.g., the two already mentioned above), so I don't see why this change should be blocked while others get a pass. If we want to be more careful about API breakages, then we should first formalize this somehow and apply the same rules to everyone.
I never considered the printed version of MLIR stable, and I'm not sure that its stability should be the goal that prevents improvements to the printing format.
Regarding churn, we should not forget that not improving the printed format also causes churn every time someone is reading or modifying it.
I think that redundancy in IR causes more issues than it solves, so I would prefer landing this change.

This is an unnecessary change that just causes downstream churn. Not sure it is worth it

Looking at the previous discussion, I think that people's opinions differ on this one. However, I would like to touch on the phrasing of this comment. The very fact that someone spent the time to make this change means that they do think it is necessary. Thus, in my opinion, stating that it is unnecessary in such absolutist terms is disrespectful. The same point could have been achieved by saying something like "I disagree with this change because <your argumentation>". Even if you know Alex personally, and you're sure he wouldn't mind the statement, this is still a public forum where others (maybe potential future contributors) will read these discussions. I think we should be more careful in how we communicate, and make sure that this forum is an inclusive place where others' opinions are respected.

Interesting, and in this case we cannot infer the result type based on operands? Isn’t the result type always matching the outs tensors?

I think Mehdi has a good point about spending more time on design. I agree clean ups are nice, however I feel like the incremental breaking changes are causing the problem and we shouldn’t take those lightly. I believe there has been several syntax changes proposed to linalg ops in the recent past? Should we make sure we have a syntax that won’t need more change in the near future before make those changes? (Maybe it is worth a quick chat on discourse to make sure we address all the potential syntax problems at once and not continuously break users). What do you think?

Sure, then I would like to remove result types from ALL linalg ops. I can prepare the patch, if you are fine with it.

I would still highly prefer those ops be removed.

Why? If you want these ops to be removed, should we also remove linalg.matmul, linalg.fill? If not, then what's the difference?

[not a statement of endorsement of either form]

I think that redundancy in IR causes more issues than it solves, so I would prefer landing this change

The redundancy is in the printed textual debugging format, not in the IR. For me the real question is what is more readable and easy to understand while debugging for the average engineer/user (and I've done this wrong a few times and resulted with something that is nice for me to read and write but doesn't serve this [and I'll cause breakages when I get around to fixing ...]). No redundancy is good but if reading requires solving a puzzle then some redundancy is better. This case I'm not sure TBH, knowing the outs linalg convention vs general MLIR convention/mathematical notation only (and redundancy can be removed by removing types from the outs section also possible if redundancy is main thing).

Mehdi mentioned not to block improvements based on the churn it produces (and that was a general point, not specific to this change I believe). I semi agree as this is unstable debugging textual format. But I've also seen a cosmetic change requiring more than a week of around the clock SWE time (so probably 3+ SWE weeks using regular measurements) for a cosmetic change that only some liked more. The delta improvement vs the cost wasn't really worth it IMHO, but improvement is not an objective measurement and others may value that improvement higher. Now having a script or tool that does the update changes such calculations (e.g., Alex showing here how to run an update here). Thomas also has good point about many small changes, we had like >20 small breaking changes in one segment that could have just been one and done, that way at least folks real work is not broken up multiple times.

So, it looks like launching a sed command is churn. Comments are not churn, even though they take much more time and effort.

I would like to have a single format for all DPS ops (linalg/thlo/linalg_ext). I would also prefer to remove result types like it was already done for map/reduce/broadcast/transpose + tHLO ops since DPS interface ties results and outs.

and redundancy can be removed by removing types from the outs section also possible if redundancy is main thing.

Then ins operand list would look differently from outs. I think it would look/read worse.

In strong favour of this.
All IR should be readable, especially linalg, which is not the easiest dialect.

Interesting, and in this case we cannot infer the result type based on operands? Isn’t the result type always matching the outs tensors?

I think Mehdi has a good point about spending more time on design. I agree clean ups are nice, however I feel like the incremental breaking changes are causing the problem and we shouldn’t take those lightly. I believe there has been several syntax changes proposed to linalg ops in the recent past? Should we make sure we have a syntax that won’t need more change in the near future before make those changes? (Maybe it is worth a quick chat on discourse to make sure we address all the potential syntax problems at once and not continuously break users). What do you think?

Sure, then I would like to remove result types from ALL linalg ops. I can prepare the patch, if you are fine with it.

It makes sense to me to have this be consistent. But again what I mostly want to make sure is that we don't do another breaking change in the syntax in 2 weeks.

So, it looks like launching a sed command is churn. Comments are not churn, even though they take much more time and effort.

In my experience, the main problem hasn't been only the integration but the fact that it disturbs developers flow. For instance developers often store models/IR in linalg format to be able to work in isolation of the front end. Those kind of changes forces them to regenerate models.

As Jacques mentioned, readable IR is subjective and compatibility breaking changes are expensive. So improvements are great but we should treat them like other important design changes by making sure we discuss and reach a rational design that we think can be stable. This way we can point developers to when they are suggesting more changes and change it only based on new information.

As a meta-point: I'm concerned about the push-back on improving things based on churn. The comment about users wanting stability can only be an argument about spending more time on design and being more thorough before adding anything to the codebase. However that means much less velocity and higher cost to experiment. So far this isn't the tradeoff we've been making in MLIR I believe.

I am not concerned about churn. Churn is part of working at HEAD with MLIR. But doesn't mean that all churn is good. There was a lot of churn when ops were split out of standard dialect, but that was done deliberately to get to a better end state. I am not convinced this is a better end state. It is a parser/printer change that is made with the argument of increasing readability. Firstly, I am not sure it does (now you need to know that the outs operand type correspond to result type, and it only works for named ops not linalg.generics). Second, if it is not a clear win, and in absence of more justification, I would vote for status quo so that we don't subject all downstream users to have to update their lit tests for seemingly no benefit.
Saying that there are other changes that did this is not justification enough IMO. There were changes that were also blocked (Jeff wanted to make a change that added a comma to some arith dialect ops, there was also a discussion on change outs to init for Linalg ops that I wasnt sure is worth it for the same reason even if I prefer init).

I dont see any reason why linalg ops need to be readable.

Can you elaborate? I'm surprised because taken at face value, this comment is quite against the concept of custom printer in MLIR themselves!

Ok, fair point. Narrowing my scope of the comment. I think this change does not increase the readability of Linalg ops. I understand personal preference, but I am more arguing for status quo when the distinction comes down to personal preferences.

Eliding return types when they are coupled is quite the norm I believe in upstream dialects, what makes it different in your view here?

If the ops already do this, that is the status quo. I am just saying I dont think there is enough justification to change the status quo.

From my experience integrating MLIR into Google's internal repository, breakages from changes like these seem to be very frequent (e.g., the two already mentioned above), so I don't see why this change should be blocked while others get a pass. If we want to be more careful about API breakages, then we should first formalize this somehow and apply the same rules to everyone.
I never considered the printed version of MLIR stable, and I'm not sure that its stability should be the goal that prevents improvements to the printing format.
Regarding churn, we should not forget that not improving the printed format also causes churn every time someone is reading or modifying it.
I think that redundancy in IR causes more issues than it solves, so I would prefer landing this change.

This is an unnecessary change that just causes downstream churn. Not sure it is worth it

Looking at the previous discussion, I think that people's opinions differ on this one. However, I would like to touch on the phrasing of this comment. The very fact that someone spent the time to make this change means that they do think it is necessary. Thus, in my opinion, stating that it is unnecessary in such absolutist terms is disrespectful. The same point could have been achieved by saying something like "I disagree with this change because <your argumentation>". Even if you know Alex personally, and you're sure he wouldn't mind the statement, this is still a public forum where others (maybe potential future contributors) will read these discussions. I think we should be more careful in how we communicate, and make sure that this forum is an inclusive place where others' opinions are respected.

Acknowledged. Will try to phrase in less absolutist terms.

As a meta-point: I'm concerned about the push-back on improving things based on churn. The comment about users wanting stability can only be an argument about spending more time on design and being more thorough before adding anything to the codebase. However that means much less velocity and higher cost to experiment. So far this isn't the tradeoff we've been making in MLIR I believe.

I am not concerned about churn. Churn is part of working at HEAD with MLIR. But doesn't mean that all churn is good. There was a lot of churn when ops were split out of standard dialect, but that was done deliberately to get to a better end state. I am not convinced this is a better end state.

Sure, we agree here, but it seems you’re not answering to the part of my comment I expected: this was a meta point about churn-based arguments specifically. There is no question we need to focus on the merits of the change in itself! Let’s continue below.

It is a parser/printer change that is made with the argument of increasing readability. Firstly, I am not sure it does (now you need to know that the outs operand type correspond to result type, and it only works for named ops not linalg.generics).

Right: but I claimed earlier that this seems consistent with most other dialects, at least upstream.

Second, if it is not a clear win, and in absence of more justification, I would vote for status quo so that we don't subject all downstream users to have to update their lit tests for seemingly no benefit.

Absolutely: we shouldn’t change things for no benefit. Can we focus on the why you don’t think there is a benefit here?
To start with, I see at minima design consistency with the general dialect design I think I saw upstream so far.

Saying that there are other changes that did this is not justification enough IMO. There were changes that were also blocked (Jeff wanted to make a change that added a comma to some arith dialect ops, there was also a discussion on change outs to init for Linalg ops that I wasnt sure is worth it for the same reason even if I prefer init).

I dont see any reason why linalg ops need to be readable.

Can you elaborate? I'm surprised because taken at face value, this comment is quite against the concept of custom printer in MLIR themselves!

Ok, fair point. Narrowing my scope of the comment. I think this change does not increase the readability of Linalg ops. I understand personal preference, but I am more arguing for status quo when the distinction comes down to personal preferences.

Yeah I’d like to go beyond personal preference here and find some guidelines /design principles we can anchor to (now and in the future) when discussing custom syntax.
I think the best outcome here will be a new guideline documentation on the website :)

Eliding return types when they are coupled is quite the norm I believe in upstream dialects, what makes it different in your view here?

If the ops already do this, that is the status quo. I am just saying I dont think there is enough justification to change the status quo.

I don’t get your point here: this goes back to my meta point before.
If the op does not do it because it wasn’t well considered when introduced, the issue is in the original review instead of now.

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.

For instance developers often store models/IR in linalg format to be able to work in isolation of the front end. Those kind of changes forces them to regenerate models.

I think this is the only valid argument against landing this PR in this whole thread.

I am also curious why linalg was used as a storage format. Are there many clients/models that use it instead of higher level dialects?

For instance developers often store models/IR in linalg format to be able to work in isolation of the front end. Those kind of changes forces them to regenerate models.

I think this is the only valid argument against landing this PR in this whole thread.

I am also curious why linalg was used as a storage format. Are there many clients/models that use it instead of higher level dialects?

My understanding is that it allows to keep a representation of the model independently on importers/front ends.

Adding few people who had problems in the past with breaking linalg changes so that they can weight in and give more details on why having a more stable linalg representation would help:
@Benoit @harsh @powderluv

We at nod.ai ship / serve terabytes of Linalg IR everyday (and it is increasing) as part of SHARK (https://github.com/nod-ai/SHARK). We have end users on metered connections (or offline) we try our best to reduce churn for. Stable Diffusion requires shipping 6GB of linalg IR for FP16 (per model variant) and we currently support 6 variants atleast. In early December we switched back to textual representation (at the cost of storage space) to provide better stability of the IR. We understand the cost of living on HEAD and having to regenerate our IR (we regenerate all the SHARK tank models every night but if an LLVM bump fails we stay on the older IR until fixed across the stack in LLVM, torch-mlir, IREE, SHARK etc ).

We have no opinion on this PR but just want to highlight downstream usage and the cost of a IR change that trickles down. Happy to share any other useful information if it helps inform our design.

For instance developers often store models/IR in linalg format to be able to work in isolation of the front end. Those kind of changes forces them to regenerate models.

I think this is the only valid argument against landing this PR in this whole thread.

I would also hope the same consideration would be given to downstream users who have lit tests written using Linalg. I explicitly didnt use this example cause in reality shipping models in IR is not the recommended way (Linalg is not a stable IR), but writing lit tests for transformations downstream when your compiler is based on Linalg is legitimate. Hope the churn there is considered when making textual changes.

(btw, I was admonished earlier for using absolutist language in comments. Same should be applied here. All comments on this threads are from folks who have explicitly taken the time to review and participate in the discussion, at expense of other work/personal time. Please do respect the time taking by all members to review the change).

I am also curious why linalg was used as a storage format. Are there many clients/models that use it instead of higher level dialects?

Benoit added a comment.EditedJan 20 2023, 4:26 AM

I would like to second what Thomas, powderluv and Mahesh have been saying. Also, while the provided regex would indeed take care of the majority of cases, my concern is more about the discoverability here. Once you know that this change is the reason for the compiler errors you're getting, the regex means you're close to having fixed it already (although, powderluv's story shows that 'close' may still involve re-distributing multi-gigabyte files). But for an average user finding out that their usually successful commands have started to fail, how long will it take them to understand that that is the problem? The last time it happened, it took me hours in GDB, because I was getting cryptic errors and as a compiler developer, my first guess was that something was wrong in my own in-progress code. This could have been disproved easily by retrying without my local changes, but I wasn't expecting that to be broken already without my local changes. This is, I think, just the way most of us work. What would really help in instances like this is if we had better error messages. Particularly in a compatibility-break case like here, an error message explicitly calling out the breaking change, linking to this Review, such as error: as of D141804, linalg ops no longer have -> return type.

For instance developers often store models/IR in linalg format to be able to work in isolation of the front end. Those kind of changes forces them to regenerate models.

I think this is the only valid argument against landing this PR in this whole thread.

I am also curious why linalg was used as a storage format. Are there many clients/models that use it instead of higher level dialects?

My understanding is that it allows to keep a representation of the model independently on importers/front ends.

Adding few people who had problems in the past with breaking linalg changes so that they can weight in and give more details on why having a more stable linalg representation would help:
@Benoit @harsh @powderluv

So far there was no requirement for stability on MLIR dialects, and in fact they change all the time, often in significant and non-trivial ways. The pretty-printed format is not even remotely close to being a stable storage format, and has never been advertised as such. I've been on the receiving end of this churn for a long time and I'm fine with dealing it for the sake of progress. If stability has become more important for you we should have a forum discussion and ideally turn it into some kind of written policy so we don't have to repeat the same discussion over and over again on random code reviews.

This discussion in particular has a strong smell of "churn is fine, as long as *I* do it", which we should avoid at all costs.

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

When we discussed creating StableHLO as storage format for ML programs, we had a very similar discussion: Should we simply turn mhlo, our compiler IR, into the stable format and use that to store programs or was there value in separating the storage IR into a separate dialect. We went with the latter and one reason was to keep our flexibility to morph the compiler IR without impacting use cases like yours. StableHLO even provides some forward/backward compatibly, so that we can transform IR forward in an automated way (or enable scenarios like the error messages that @Benoit mentioned). This all comes at a cost but we decided it worthwhile to pay for the hlo family of dialects.

Which route should we take for linalg? We need to retain reasonable flexibility in changing the IR. Should we discuss formalizing a variant of linalg that is stable? Or would it be possible to migrate your usecase to a format that already has certain stability guarantees (StableHLO, TOSA, ...)? For the former, we should start a discussion on discourse. For the latter, the IREE team might be more appropriate to advise.

Also @nicolasvasilache for visibility.

@herhut the reason we chose Linalg like @ThomasRaoux mentioned is because it gives us an abstraction from the frontend and the backend. Currently in SHARK we lower from various frontends TF, JAX (HLO), PyTorch (torch-dialect), Tflite->TOSA into Linalg and then we target whatever backend the end-user wants to use. We could technically choose one of the higher level IRs like StableHLO and then write translators to other IRs but in the end they all come down to Linalg anyway for us (we only codegen). We want the ecosystem to grow and not be burdened so we are willing to take on any "churn for progress" whenever they happen. Some stability would be great as it evolves but I think that will happen as it matures. We, at least for our usecase don't need backward / forward compatibility yet. Happy to discuss more. Thanks for hearing our usecase.

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)

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"?

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..

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.

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..

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.

@mehdi_amini and there is another aspect you can see in these examples that we have been kicking the can on: there is a strong need for a generic mechanism to represent sequences of these things (i.e. kronecker3 wraps 3 linalg.generic).
IREE has started creating new one-off named ops (softmax and flash_attention so far) and it really simplifies parts of the system.
Without a generic mechanism however, these require large amounts of 1-off C++.
The good thing though is that things compose hierarhically (i.e. softmax is messy but flash_attention is softmax(matmul(softmax))-ish).

Would be thrilled to discuss these aspects deeper too with you if you're interested.

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)

We were also crossing project boundaries both updating to LLVM at a different pace. We needed torch-mlir to be updated to the latest LLVM release (and that was delayed by 2 weeks though it is on a weekly cadence which usually roughly lines up with IREE LLVM updates). We had to ship something that week so textual IR seemed to work. We have since switched back to bytecode IR. One other thing to note is that though the bytecode was supposed to be 4x smaller than the textual IR we currently only see a ~2x reduction (3.3GB Unet got to 1.6GB ). We haven't gotten to investigating why we couldn't get everything in bytecode but if you look at the IR (https://storage.googleapis.com/shark_tank/latest/unet64_512_512_fp16_stabilityai_stable_diffusion_2_1_base/unet64_512_512_fp16_stabilityai_stable_diffusion_2_1_base_torch.mlir) we still have a lot of text. Also another factor to consider having it in byte code reduced compile times by 2x on the system I tested on.

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..

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.

Lots of discussion on this thread that I am not responding to in this comment -- I am just narrowly responding to thing thing I was plussed in to. Having done some of the ergonomic work, I think that some of this is a bridge too far to expect from a low level compiler IR and I've not personally seen the benefit emerge to go to an end state of a pure math representation in the abstract. I'm +1 on readability and compactness improvements, especially if they are with some design thought put in that gets them closer to a good end state and aren't just going to oscillate a bunch of times based on preference. Churn is the price we pay to move forward. I would like to actually move forward, though, not just oscillate on preferences or non goals, and that is how I evaluate things.

I think the "mathy" and C++ vs DSL preference for op definitions is orthogonal to this discussion and, in hindsight, may even be a non goal for this layer of representation. It feels like something that would be much better homed at a higher level, personally. The core IR does not need to solve all concerns in such a system, imo.

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..

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.

Lots of discussion on this thread that I am not responding to in this comment -- I am just narrowly responding to thing thing I was plussed in to. Having done some of the ergonomic work, I think that some of this is a bridge too far to expect from a low level compiler IR and I've not personally seen the benefit emerge to go to an end state of a pure math representation in the abstract. I'm +1 on readability and compactness improvements, especially if they are with some design thought put in that gets them closer to a good end state and aren't just going to oscillate a bunch of times based on preference. Churn is the price we pay to move forward. I would like to actually move forward, though, not just oscillate on preferences or non goals, and that is how I evaluate things.

I think the "mathy" and C++ vs DSL preference for op definitions is orthogonal to this discussion and, in hindsight, may even be a non goal for this layer of representation. It feels like something that would be much better homed at a higher level, personally. The core IR does not need to solve all concerns in such a system, imo.

Good point re. "The core IR does not need to solve all concerns in such a system, imo."
This does point towards higher-level layers indeed, thanks for sharing your insights!