[Mlir-commits] [mlir] Remove Pure attribute from Linalg::IndexOp. (PR #68894)

[llvmlistbot at llvm.org]

MaheshRavishankar wrote:

> > I don't see why linalg.index should not be pure. It has no side-effecting semantics.
> 
> We had this discussion a few years ago, everything is already explained above in this issue!!
> 
> In the current design of linalg.index there must be a side effect: a pure operation always return value computed from its arguments (i.e. a constant when there is no argument).
> 
> To model the transfer of information from the enclosing linalg.generic to the linalg.index operation, you need a "channel" so either:
> 
> * There is a hidden resource used to communicated the information: linalg.generic creates/write this resource and linalg.index reads from it (hence it isn't pure).
> * There is a SSA value carrying this state (can we get back something inspired by the linalg.indexed_generic design with a block argument?), in which case linalg.index could be pure.
> 
> There is no other option within the design of MLIR today, I floated [above](https://github.com/llvm/llvm-project/pull/68894#issuecomment-1849323082) that we could explore a different kind of model by introducing a concept of "region instance" to make the effect "local" there, but that requires some very careful design work. In the meantime: linag.index in its current form must have a side-effect.

I agree with all your points, but making the op not pure is not the right solution. It is a pessimizing solution cause it is adding a side-effect that does not exist (at least that is not what the ops intent is). I agree this op is ill-defined because of the reasons you mentioned.  As far as I can see
1. "region instance" sounds like the right setup. I am not sure you linked to the right comment, but what I was thinking was to say that there is an op that is valid only if it defined within a region.  So it can be CSEed/moved around within the region, but cannot be "hoisted" out of the region for example. I agree this is a big change.
2. I think one solution is to introduce a single trailing block argument to Linalg operations. The `linalg.index` can take this block argument as an operand to establish the region dependency. Its not the cleanest solution, but is the least disruptive cause it should only require changes to the verifier and lit tests. 



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