This is essentially just pushing the cost of fragmentation downstream. This makes sense to me if there are distinct non-overlapping sets of maintainers, which is arguably not the case today, at least in MLIR. And the cost for the same set of folks to maintain multiple projects with dependency chains sounds higher. To be blunt and selfish, if I had to start a separate glue project around MLIR, what would be the incentive for me to contribute back anything? I would be also concerned if we refused the in-depth technical discussion because of high-level principles, otherwise we would be encouraging people to form camps (that we donât necessarily know of and canât influence).
It feels like the builders discussion is challenging because it falls into the grey zone between style or paradigm changes and functionality changes. Adding a new backend or a new dialect wouldnât be much of a problem, even if it uses additional abstractions inside. Making them available to others starts to get interesting.
Let me put this back into context. Disregarding all the other arguments, is LLVM API using builders a sufficient reason for MLIR not to explore anything else?
Such âexpansionâ transformations are much more common in MLIR due to its multi-level and progressive nature than they are in LLVM proper where most of the expansion actually happens in frontends, each of which can adopt its preferred way.
There are certainly costs in having a larger API surface, namely maintenance and entry barrier, but I am not convinced why we should accept this costs for the APIs that manage transformations (direct IR manipulation vs. DRR/simple rewrites vs. dialect conversion) and not for APIs that implement transformations (builders).
This is essentially just pushing the cost of fragmentation downstream. This makes sense to me if there are distinct non-overlapping sets of maintainers, which is arguably not the case today, at least in MLIR. And the cost for the same set of folks to maintain multiple projects with dependency chains sounds higher. To be blunt and selfish, if I had to start a separate glue project around MLIR, what would be the incentive for me to contribute back anything? I would be also concerned if we refused the in-depth technical discussion because of high-level principles, otherwise we would be encouraging people to form camps (that we donât necessarily know of and canât influence).
Fwiw, with llvm-aligned frontends like npcomp, we are running straight into this for the simple pragmatic reason that I have little desire to do it again in the hard/brittle way that tf2xla was done (same problem: reducing an open set of thousands of âsubroutineâ style ops to dozens of primitives). My sense is that some kind of macro expander and associated DSL needs to exist, and that is at a higher level than any of the builder discussions (and not in conflict with them). Being a âfrontendâ top level project, I think it is proper for it to be trying to build tooling that increases itâs efficacy, and I suspect that some of that will eventually be general enough to promote out in some form if desired.
I really appreciate all of the work to increase the usability of the builder APIs and I think what I could benefit from the most is a layer above this for solving a different level of complexity. Iâd like to not see 15 divergent attempts at such a thing in sub-projects, but I also am not sure we know what we need enough yet to be trying to layer new abstractions into the core APIs. You have more visibility into the other projects that exist and maybe this is clearer for you?
With API instability and the difficulty of keeping up with upstream, I donât really see such approach having chances of success or lead to a widespread adoption. It risks instead having every client forking for their own convenience, improving locally, and not contributing back.
But more importantly, if this is about downstream clients that are shared by the upstream developers, it seems to me that we would make our own lives harder for a reason that I havenât totally grasped yet.
(Also in this case there is the technical aspect that weâd need to make ODS and the generator externally extensible in the first place to achieve this out-of-tree).
My take is that if there are utilities or tools that are âgeneral enoughâ to be shared by multiple/most
downstream project: having them in-tree is what ensures the best maintenance path as well as the most chance of having something well integrated and consistent across the project.
The EDSC suffered from being âthat thing on the sideâ previously, I the current proposal as a good direction to unify back things behind a single API that integrates well with ODS. I donât see us removing EDSC without a good alternative right now, and just for this Iâm very supportive of Alexâs work here.
We may have a fundamentally different viewpoint on this. I see fragmentation as inevitable.
LLVM needs to have a single consistent coding style, this is really important for project management, allows us to scale the codebase better, and makes it easier to explain LLVM to new people. Iâve fought this problem (only semi-successfully!) for years when thing like LLDB got merged in etc - LLDB having a âhouse styleâ that was significantly different than LLVMâs was a major issue that reduced cross pollination. This argues that we should make sure the LLVM standards are high quality, consistently apply them, and improve them when needed.
That said, I see zero chance that the world will adopt LLVM standards uniformly, no matter how great they are. This means that there will always be downstream fragmentation. The argument youâre making is that we should privilege one or two of those fragments and merge those into LLVM - that doesnât work, because now you have internal fragmentation and canât manage the LLVM project effectively. It also means that other fragments will want first class support.
There are analogies to this in LLVM subprojects. Early in the project, we struggled with âwhy not merge my research project into LLVMâ? There were many examples of this including VMKit, âHLVMâ, TVKit, and others. We eventually decided that it was more important for the code base to be high quality and actively maintained than for it to include âall the exciting things that might want to work outâ. As you probably know, weâve recently added an LLVM Incubator process as a sort of relief valve to enable a new state.
I agree with you. What Iâm arguing is that we should take the style issues first, resolve those w.r.t. the existing builders, and see what is left to discuss. This is why I say things like âthere needs to be a right answer: are locations always specified, or not?â. Whatever the answer is should be consistently applied to both. Once we decide, we roll it out and have reduced one deviation, and then can discuss the next topics with less ambiguity, etc.
Iâm not sure what you mean here. Are you saying âwe have OpBuilder, does that prevent having a higher level abstraction in tree?â. If so, I would say âno, it doesnât prevent it categorically, but we would look like it as any other infra extension to llvmâ. The problem with EDSCâs (as we discussed) is that they were landed early in the cycle of MLIR where a lot of researchây stuff was underway. If we didnât have them and someone proposed their inclusion today, I would be strongly opposed to them for all the reasons Iâm explaining above.
I agree that the risk of ânot contributing backâ is real, but I donât see how this discussion relates to it. I donât think that EDSCs are a significant hinge that will affect this one way or the other.
@ftynse, with regard to the things mentioned under âInteraction with EDSCâ, I wanted to check what happens to code like the one appended below from Linalg thatâs using not just different builders but a lot of abstractions and new patterns around it. Could anyone even guess from the code below that itâs actually creating load and store ops?! Would such things go away with this proposal? (progressively or at once?) I feel parallel patterns like these are a big hindrance for readability - not just for new developers but primarily even for other MLIR developers! Even the most active MLIR developers would have to dig deeper to figure out itâs creating two load ops and one store op.
IndexedValueType output(op.output());
IndexedValueType input(op.input());
Value lhs = output(indices.outputs);
Value rhs = input(indices.inputs);
...
output(indices.outputs) = maxValue;
I strongly feel patterns like these shouldnât have been allowed in the first place upstream.
Iâm curious to know what these are - if you could just summarily list them. Iâve only heard about the EDSC used mainly by Linalg and Vector, and Iâm a -1 on these abstractions for all the reasons mentioned upthread. Replacing these completely with whatâs proposed here or some form of it would still be a net positive move IMO. But itâs still not fully clear to what extent that would happen - the statements at the end of âInteractions with EDSCâ are a bit concerning.
I feel the proposal could benefit from an allude to how the higher order builders proposed could be taken even further. The benefits of IDE/clangd autocompletion, prompts, inline errors are really great to have for productivity, but those could be achieved with what @jurahul suggests above or a lower order form of this proposal, in which case they could just replace standard builders. This is all assuming clangdâs code completion canât be made to work with such function templates. If it can, the standard builders could just be retained. Separately from this, I think we should aspire for something even âhigher orderâ if we want to add a new builder abstraction upstream â because if the code completion part is tackled by changing the standard builders slightly, I feel the proposal is adding a thin layer or not spelling everything out.
As a general observation, MLIR positions itself as a partly productivity-oriented compiler framework. If one of the original goals was not having to write n-th CSE implementation for each compiler (see early presentations from Chris), I donât see why we at the same time encourage users to write n-th IR âexpanderâ.
I donât know what folks need in general, my proposition is abstracting from what we have in-tree and several TF-related projects. By putting this up for discussion, I hoped to receive some feedback on what else is necessary or simply useful.
As a matter of fact, I agree that fragmentation is inevitable. Therefore, I am trying to find ways to minimize its cost.
Maybe we can actually reduce this discussion to what is considered to be LLVM standards and which parts of the proposal contradict those standards, so we can discuss those parts separately.
I donât fundamentally disagree with this. But there is cost to everything, roll-out duration and api churn will affect most users here.
LLVM and derived projects (including translation in MLIR) uses llvm::IRBuilder API to construct (most of) the LLVM IR. Should MLIR adopt the same API pattern, i.e. Builder, only for the sake of having the same API patterns as LLVM (see âstandardsâ above).
They were also landed because they were, and still are, used. Thatâs why I am trying to converge EDSC and Builders and reach a state where we have one thing that is good enough for everyone. The alternative at this point is to keep the status quo.
Weâve gotten so meta in this discussion that I would like to see what are the ways of mitigating the risk, regardless of EDSC or this proposal
As you probably know, EDSC is a combination of many different things. I am trying to progressively integrate them with the rest of the infrastructure as explained in Evolving Builder APIs based on lessons learned from EDSC. In his posts above, Chris argues that even the current proposal is too broad. Considering such helper structures is definitely out of scope here. For practical purposes, such helper structures will likely remain unchanged in the first iteration and have their constructors updated to take a builder if we decide to remove edsc::ScopedContext. Letâs not derail this discussion even further though.
To be honest, I donât understand how affine structures are modeled in MLIR without digging deeper, and I would guess neither does the majority of the contributors. This does not preclude them from being in tree.
I listed some examples in my previous answer, each word is a different link:
I canât even imagine the amount of pushback Iâd would have received had I proposed to replace builders and EDSC by a completely new API; nor the amount of work to do. If we want progressive change that does not introduce more divergence, components need to interact with each other.
It seems like both you and @stellaraccident have some ideas about higher-order builders. I am interested to hear them and potentially incorporate them in the proposal.
The signature of the function is template <typename OpTy, typename... Args> OpBuilder::create(Location, Args &&... args), which means it accepts just about any argument list. While I think itâs technically feasible, although not trivial, to discover that the argument pack is forwarded through another function template with variadic template parameters to a method on a dependent type, and use the list of possible overloads of that method as helper prompt, I would consider this to be too niche to be included in clangd. Even if it is included, it only gives you the documentation, implicit conversions are not triggered (they are in @jurahulâs proposal and in mine).
Iâm not referring to internals or anything specific to the dialect, but plainly to IR building - something that is common ground for all MLIR developers that they shouldnât have to dig deeper to understand. The code snippet I showed from Linalg Transforms (again reproduced below) is creating load/store ops as you know â this isnât anything dialect specific, and yet itâs unreadable to me. (The IR creation code isnât clearly spelt out due to the metaprogramming and lost/mixed among regular code.) IR constructing methods/code in other dialects upstream arenât like this.
IndexedValueType output(op.output());
IndexedValueType input(op.input());
Value lhs = output(indices.outputs);
Value rhs = input(indices.inputs);
...
output(indices.outputs) = maxValue;
Thanks Alex for taking the time to put these examples together!
Speaking personally, the âcurrent proposal w/o createâ reads best to me. I like that it is succinct and reads very similarly to the corresponding textual .mlir (thereâs something about the . vs edscâs _ after the dialect name which makes it much easier to read). The create is just line noise once we get to this level of fluency.
For the âeven more DSLâ version, it is too difficult to see what concrete ops are being created.
I would really like to have a âcurrent proposal w/o createâ-like API available for these sorts of âexpansionâ patterns.
These are quite useful. Did you paste the wrong link for the first one? There is no create there - the builder is just named create. I assume you meant to have it use createCmpI etc.
If these are the current proposals, Iâm not sure why the proposal says standard builders are going to stay. With either of the first two options listed by @ftynse, one can just replace the standard builders. I assume passing an explicit location will also be supported if needed. I felt that was the confusing part of the proposal - the first post neither says standard builders would be replaced nor deprecated and replaced over time.
I really donât understand what you mean here. MLIR already has OpBuilder - we are not talking about removing the presence of builder APIs! I think youâre arguing that the existing builders are not good enough - Iâm arguing that we should either improve them or delete them. There should be one builder abstraction that is âgoodâ.
FWIW, I donât agree that the create-less APIs are âgoodâ. Beyond the problems with naming conventions, I am very concerned about their reliance on tblgen code generation is also suboptimal â because it means that anything built on top of the builders need to be similarly code generated (they canât just use C++ templates). This is one of the reasons that I canât see this ever replacing the existing builders. Given that, this direction cannot lead to unification behind a single âgoodâ API.
Let me give you one example of what I mean. With the existing builder APIs, a downstream project could say âhey, I want a builder that does extra stuff when I build operationsâ (e.g. maybe make the location implicit). RIght now, you can do this easily:
You canât do this sort of thing when you rely on tblgen code generation for the builder APIs - youâd have to tblgen codegen ImplicitLocOpBuilder, which would require adoption by every dialect that wants to participate.
This breaks scalability of the ecosystem, which is a critical issue to me. I can see how this approach can be used for a couple of dialects, but unless it can be âtheâ thing, I have a hard time getting excited about forking and fragmenting the ecosystem.
Furthermore, EDSCâs have not been widely adopted outside of the code that ntvâs code, so I donât see strong empirical evidence that there is a âpullâ for this outside a couple of specific (and very amazing) engineers that prefer it.
Brainstorming here: What if builder.setInsertionPoint*() took a code point and a Loc. Then each create<> call wouldnât have to specify the loc explicitly? It might be more awkward for pieces of code that are operating on large numbers of OPs, but we could also keep the existing create<> calls that take a loc for those cases?
No, I did exactly what I intended. I do not propose the createCmpI version, Chris does and then explains why that version isnât a significant improvement over the current state.
Because the current proposal would be using builders internally. I have not considered fully replacing them, yet, as the proposal is large and contentious enough already. For example, it is not fully clear to me what will happen with various type- and attribute-constructing functions currently in Builder (although this creates a disparity in the API between âprivilegedâ types that have Builder API and dialect-specific types that do not), with PatternRewriter and related functionality, and with the actual ::build functions that currently take an OpBuilder.
That being said, if not removing builders is the main blocker for going through with this proposal, letâs discuss that as well.
MLIR also has the possibility of populating an OperationState and calling Operation::create to construct operations. It was not removed despite having builders. I donât see why the same should not be allowed for one more API abstraction level.
I agree this is a concern. However, we already generate a lot of APIs: interfaces, passes, accessors to Op operands/attributes. Realistically, how many of such builder extensions can one come up with? Caching locations is one good example, and also one of the motivations for EDSC If there another where one wants to modify the op creation behavior? If one only needs to be notified that the op is about to be create or has been created, this is already possible through the listener, no need to wrap the builder in such cases.
Iâve seen folks attempting to use it in HLO and then getting asked to remove it during code review. So the visible lack of adoption is, at least partly, due to the pushback against adoption. Note that I donât propose that everyone just adopts EDSC, and that I listed several examples above where similar API patterns were added locally.
Assuming we donât get hit by overload resolution+templates, this would still be subject to the lack of autocomplete/documentation and lack of implicit conversions for a small decrease in verbosity.
Let me flip this. What would be the alternative that meets the following requirements:
minimal redundancy in subsequent calls (e.g., no repeated location, ideally no repeated meaningless prefix);
I see what youâre trying to show, but these serve two different purposes. The OperationState is the underlying implementation details that power the infrastructure in a âtype erasedâ way. The OpBuilder::create<> template is the typed layer that is intended to be used by users of the MLIR APIs.
The EDSL stuff is another âtyped layer that is intended to be used by users of the MLIR APIsâ. That is why it is redundant, and only one should stand. If the ultimate approach is to use the EDSL inspired API, then it should be built on top of the OperationState API directly, there isnât a clear reason it should go through another layer of indirection.
I donât understand what youâre saying here. Surely you agree that there is a difference in significance between ops and interfaces/passes/accessors. Ops are the primal thing in MLIR and exposed by everything, building generic code over families of them is extremely common. We should be doing more here to make this easier, not less.
I think youâre arguing here for a monolithic builder design that ties all concerns into one API. I donât think this is a good or scalable approach - we should go for simple things that can be layered/composed on top of each other.
Fair point, but I think we should resolve that by deciding whether it is the right thing to do and apply it uniformly. The signal youâre seeing above is just an indicator that people are not happy with EDSCs. If that is the case, then the right answer is to just replace all uses of them with OpBuilder in tree and remove EDSCs (or sink them into other MLIR derived projects that want them). I would also be very +1 on this, it combines very nicely with the orthogonal goal of âimproving OpBuilderâ.
I donât understand this âflipâ at all. Youâre getting pushback on this thread about various design points youâre proposing to achieve those three goals, so it isnât clear that we âhave toâ solve those problems.
I love that youâre pushing the envelope here to try to resolve the fissure between OpBuilder and EDSCs, but I am fairly serious that we should consider the alternate path: just remove EDSCs (replacing with direct use of OpBuilder), and then continue a discussion of improving OpBuilder. The fork in the universe is really a separable problem from the âhow do we get the best builder API possibleâ.
I canât see any strong argument that we should remove OpBuilder and keep EDSCs given the compile time and many other pushbacks that people have raised.
Hereâs another idea on how to make incremental progress here:
It has been observed that some use-cases would benefit from a builder that makes the location implicit at the create site: the location is a mutable part of the state in the builder itself. This is controversial because some of us want to make sure that location processing is thought through and would like to put some burden on compiler engineers to do the right thing with locations. OTOH, it is clearly useful for specific design patterns, including entire passes that work in a structured way for which this makes sense, as well as the primary EDSC use case of âsplat out a bunch of IRâ which presumably comes from one location.
I think we can have the best of both worlds here with the existing opbuilder stuff. I think we keep OpBuilder the way it currently is - it supports creation of arbitrary IR nodes, requires the location to be explicit in the create call, and is the thing that is generally passed around between the core parts of the compiler.
To solve for the use-case above, we introduce the new ImplicitLocOpBuilderdescribed above. This allows clients with the structured creation logic to opt-in to implicit location handling.
I implemented the ImplicitLocOpBuilder concept in CIRCT and weâve been using it for some time now. It works well, so I think it would be useful to upstream it to MLIR. Here is a patch that does so, Iâd appreciate feedback and review, thanks!
