[Mlir-commits] [mlir] [mlir][scf] Extend consumer fusion to multiple tilable users (PR #111955)

[llvmlistbot at llvm.org]

MaheshRavishankar wrote:

> Thanks for your time! This formalization is fairly constructive and makes the solution more clear. I have some question before reimplementation.
> 
> > The issue is that
> > ```
> > ... = firstUserOfLoop
> > 
> > ... = defnOfConsumer
> > ```
> > During transformation, the loop is moved just before the `defnOfConsumer` (note that it is unnecessary to say "lastDefOfConsumer", there is only one def). and this movement causes use-def chain violation.
> 
> As for statement `there is only one def`, don't we need to check each defineOp of all operands of consumerOp? Please correct me if I have misunderstood..

Ok, so thats what you mean by last defn of consumer... `defnOfConsumer` dominates all definitions of. You typically get it through program slice.
 
> > 2. Compute a backward slice of `defnOfConsumer` (with the op included) but cut the slice to not include any operation that `firstUserOfLoop` already dominates.
> > 3. Move the slice computed (including `defnOfConsumer` before `firstUserOfLoop`.
> 
> I am sorry that I need to figure out what `defnOfConsumer` exactly represents here before grasping the details here.
> 
> > 1. Check that `firstUserOfLoop` and `defnOfConsumer` are in the same block
> 
> There exists one corner case. As I have already commented in the current change, it is possible that one of defOfConsumer is outside the block of `firstUserOfLoop`. E.g.
> 
> ```
> %a = ...
> {
> %loopOp = scf.for
> ....
> %b = consumerOp ins(%loopOp, %a)
> }
> ```
> 
> where `%a` dominates the block of `firstUserOfLoop` ==> it must dominate `firstUserOfLoop` ==> we can skip subsequent check instead of bailing.

I am not sure I follow this example. The `%loopOp`s first use dominates the consumer. We dont have to have `scf.for` dominate all definitions of the operands of `consumerOp`, i.e we dont need the `scf.for` to dominate the entire slice of `consumerOp`. What I meant was that if you take a program slice of `consumerOp` which is cut-off at `scf.for`, the only use should be `consumerOp`. 

> 
> > 3. The first user of loop should not be in the slice computed. If it does, then bail/look for next consumer.
> 
> Based on current consumer dispatching i.e. which one of multiple consumers would be fused. it would always grab the first user among the use list. So, could we resort the use list order by `shuffleUseList` to put the next consumer ahead?
> 
> BTW, during the transformation, the order of operations generated before fusion maybe changed. I am afraid if developer have some strong assumption or expectation on these order. How about add another option like `bool intrusive`(or any other name) to control this behavior. If the user does not want to modify any order caused by fusion, we just bail rather than move those def.

I'd push back against anything that relies on program ordering of operations (unless there are explicit barrier instructions etc. which we might not support for first implementation cause you might be really able to fuse across that anyway). Only valid ordering is use-def chain, but I am fine with having a flag that gates changes of instructions order.

> 
> Looking forward your opinions, thanks!



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