Skip to content

Conversation

@makslevental
Copy link
Contributor

@makslevental makslevental commented Oct 27, 2024

This PR refractors MLIRTableGen to me more like all other MLIR tools and exposes functions such that they're reusable by downstream.

See #92709 for previous discussion.

@github-actions
Copy link

github-actions bot commented Oct 27, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@makslevental makslevental changed the title Move tblgen v2 [mlir][tblgen] Expose more of MLIRTableGen as library Oct 27, 2024
@makslevental makslevental marked this pull request as ready for review October 30, 2024 02:06
@makslevental makslevental requested review from Groverkss, ftynse, joker-eph, jpienaar and stellaraccident and removed request for antiagainst and kuhar October 30, 2024 02:06
@joker-eph
Copy link
Collaborator

I haven't looked at the implementation, but I don't think you really followed up with the discussion about the intent we aim to have behind all this? (started in #92709)

@makslevental
Copy link
Contributor Author

I haven't looked at the implementation, but I don't think you really followed up with the discussion about the intent we aim to have behind all this? (started in #92709)

The intent is to enable downstream users to reuse the functionality that is currently inaccessible by virtue of not being exposed in headers.

@River707
Copy link
Contributor

I agree with @joker-eph here, I don't yet understand the goal for all of this code movement. It's not clear to me what this helping to achieve, and the original discussion is still very relevant.

@makslevental
Copy link
Contributor Author

I agree with @joker-eph here, I don't yet understand the goal for all of this code movement. It's not clear to me what this helping to achieve, and the original discussion is still very relevant

The prior reluctance/apprehension was about this move somehow diluting the canonicalism of ODS:

the risk is that ODS loses it's aspect of being a clear spec

Which does not make sense for upstream to dictate - if someone somewhere (downstream) decides to implement a "custom backend" for ODS, I don't see how that affects upstream backends in the slightest. In addition there's no current barrier today to this anyway because one can simply use the JSON backend and generate therefrom. This change simply enables users to take a slightly more sane approach.

@River707
Copy link
Contributor

I agree with @joker-eph here, I don't yet understand the goal for all of this code movement. It's not clear to me what this helping to achieve, and the original discussion is still very relevant

The prior reluctance/apprehension was about this move somehow diluting the canonicalism of ODS:

the risk is that ODS loses it's aspect of being a clear spec

Which does not make sense for upstream to dictate - if someone somewhere (downstream) decides to implement a "custom backend" for ODS, I don't see how that affects upstream backends in the slightest. In addition there's no current barrier today to this anyway because one can simply use the JSON backend and generate therefrom. This change simply enables users to take a slightly more sane approach.

I'm not sure I agree here: upstream does dictate the best practices for interacting with the broader project via ODS, what things are supported and how, and in general the development and evolution of ODS. We may decide to change/remove/evolve in any number of ways that will break downstream significantly (with little to no support). That has been a good thing in general IMO, and prevents divergence in what it means to "define" something in ODS. It doesn't prevent someone from doing something, but it is a reasonable deterrent from going off of the supported path.

@makslevental
Copy link
Contributor Author

makslevental commented Oct 31, 2024

There are some subtle shifts in force here

upstream does dictate the best practices for interacting with the broader project via ODS, what things are supported and how, and in general the development and evolution of ODS. We may decide to change/remove/evolve in any number of ways that will break downstream significantly (with little to no support). That has been a good thing in general IMO, and prevents divergence in what it means to "define" something in ODS. It doesn't prevent someone from doing something, but it is a reasonable deterrent from going off of the supported path.

The truth is that currently anyone interested in using MLIRTableGen as a library (rather than as vendored sources) is firmly blocked by this partitioning from much of the functionality.

But nonetheless, if this is a major blocker for the languages incubator then I'm comfortable abandoning it and working on a solution/approach that lives entirely in the incubator.

@joker-eph
Copy link
Collaborator

(I apologize: still haven't reviewed the patch, but it's not a small one I need to find enough time to dedicate to it, maybe this weekend!).

The intent is to enable downstream users to reuse the functionality that is currently inaccessible by virtue of not being exposed in headers.

I get that...
The thing is that these were intentionally not exposed. That does not mean it was the right thing or that we should not expose them of course, but in general when changing course of something that was intentionally set up in one way, I rather make sure we're building some consensus on the new direction.

The main concern as far as I remember (and unfortunately this kind of intentional design choice are never documented, sometimes we can find them in Discourse archives, sometimes...) at the time was that people who start writing new table gen backends for anything (we have kind of this problem upstream with SPIRV, and at some point Linalg folks were looking into this as well), while ODS itself had been defined in a very coupled way to the C++ backend. Not having people reach out to custom backend meant that we had to improve ODS itself (and the C++ default generator) to support new features that were requested. Had we just exposed this all from the beginning we'd have many "dialects" of ODS (worst case: each dialect comes with its own TableGen backend for its own customizations...).

Now at this stage of the project, and seeing the maturity of ODS, these concerns are maybe not warranted anymore and we may just be fine relaxing this all.

This is more of what I was looking for when asking what is our intent with ODS.

I can also misremember things and omit some elements: a lot of it were in-person discussions with @jpienaar and @River707 so they may correct me or add more elements here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants