[Mlir-commits] [mlir] [mlir][linalg] Extend `FuseElementwiseOps` pattern to work with named ops (PR #144922)

[llvmlistbot at llvm.org]

srcarroll wrote:

> I suspect we will need @llvm/mlir-area-team on this.
> 
> My personal opinion is that the change may go through as is, though we may need some notice-related process around it. LLVM _de facto_ does not have any guarantee on C++ API stability. I don't think this was ever codified in the policy, but was discussed repeatedly, e.g. here: https://discourse.llvm.org/t/explicitly-spelling-out-the-lack-of-stability-for-the-c-api-in-the-developer-policy/55952. I will go as far as claim that this is an incentive to put code upstream to shift the update burden. I am also rather reluctant to bloat upstream API with similar-but-different calls.
> 
> That being said, since all contributors work on MLIR because they use it downstream, I think it is a common courtesy to minimize extra workload for everyone by staging the change when reasonable. The upstream committer may have to extend extra effort, but they will ultimately save time if everyone else does the same. Usually we do it through a PSA on Discourse that points to the commit, explains how to adapt to it, and provides some time frame after which the change will be effective. This may mean that PR is approved without landing immediately, or the main implementation lands but a switch for the default behavior is change after some period dependent on the complexity of the change. Maybe we need to adapt this policy https://llvm.org/docs/DeveloperPolicy.html#making-potentially-breaking-changes and establish a vendors team for MLIR to get relevant folks notified.
> 
> An alternative suggestion is to (1) land this with the default control function disabling fusion of named ops, (2) post a PSA on the forum that the default will change to fusing named ops in ~2 weeks given the change is rather minor, (3) resolve any comments and land the switch for the default. If there are _other_ downstreams than the one @MaheshRavishankar maintains that have concerns, they should chime in. If they don't put in the effort of doing so, why should we put the effort to adapt to them?

@ftynse Thanks for you input. I'm certainly amenable to making it a smooth transition

> On the change itself, the rationale https://mlir.llvm.org/docs/Rationale/RationaleLinalgDialect/#progressive-lowering-dont-lose-information-too-quicklya-nameprogressive_loweringa explicitly lists fusion as a transformation that drops high-level information, which can be interpreted as a desire to give users control over generalization behavior during fusion. This would be aligned with @MaheshRavishankar's suggestion, though not because it breaks downstreams but because it would better reflect the design rationale of linalg. The control function does so, but maybe we can have several pre-defined for an easy switch?

I'm still not quite seeing the benefit of the suggestion, but also not dismissing it yet.  I still need to look into it to understand what it does for us.

I'm fine with adding some pre-defined control functions.  Obviously one of them would be for only fusing generics so people can keep previous behavior if they want.  Are there any other noteworthy ones?  Happy to hear some suggestions on this

https://github.com/llvm/llvm-project/pull/144922