[Mlir-commits] [mlir] [mlir][affine]introduce AffineSymbol trait and use it for using gpu.threadid op in the inner loops. (PR #118478)

[lonely eagle]

linuxlonelyeagle wrote:

> > > > I designed AffineSymbol at the beginning by referring to constant's properties for design.
> > > > 1.The ops should be pure.
> > > 
> > > 
> > > But `gpu.thread_id` not pure! It's not marked so.
> > > > 2.Yes.
> > > > 3.Yes.
> > > 
> > > 
> > > Why can't an `AffineSymbol` trait op as proposed here be allowed to have operands which are also in turn valid affine symbols? Can we generalize/extend this?
> > > If "zero operand" is a hard implied property, the trait verifier should be checking for it, but it isn't in the PR.
> > 
> > 
> > You are right, pure and no side effects should be correct.
> 
> Now, thinking more about it, you don't really need the `AffineSymbol` trait for your use case as well as others to make the current definition more powerful. How about allowing these: "any index-typed result of a pure operation that has operands that are in turn symbols" (zero operand pure operations generating index will be a trivial case of this) as a valid symbol? This is in line with the symbol concept as a Value that doesn't change in the region of interest. The fact that the operation is pure also means it can be freely hoisted/canonicalized to the top level of the AffineScope or higher making it a valid symbol as per the existing rules as well.
> 
> This will cover gpu.thread_id any many other valid use cases without need a new trait/interface. A new trait/interface may still be needed for other cases of interest, but we should do the former first instead of having to ask other external ops to add a new trait when not needed. However, if gpu.thread_id is argued to be not "pure" for some reason, we'll have to think about the trait as proposed in this PR.

"any index-typed result of a pure operation that has operands that are in turn symbols" (zero operand pure operations generating index will be a trivial case of this)，I think it makes sense, but I have a question.Why does the operand of such an Op also need to be a symbol?
I simply modified the logic of judging whether it is a symbol to the following, but I have not yet judged the operand. I'm afraid the impact of doing so would be too great.
```
  if (isPure(defOp))
    return true;


********************
********************
Failed Tests (17):
  MLIR :: Dialect/Affine/SuperVectorize/vectorize_reduction.mlir
  MLIR :: Dialect/Affine/invalid.mlir
  MLIR :: Dialect/Affine/load-store-invalid.mlir
  MLIR :: Dialect/GPU/transform-gpu.mlir
  MLIR :: Dialect/Linalg/convert-conv2d-to-img2col.mlir
  MLIR :: Dialect/Linalg/tile-and-fuse-tensors.mlir
  MLIR :: Dialect/Linalg/tile-conv.mlir
  MLIR :: Dialect/Linalg/tile-indexed.mlir
  MLIR :: Dialect/Linalg/tile-to-forall.mlir
  MLIR :: Dialect/Linalg/transform-op-pad.mlir
  MLIR :: Dialect/Linalg/transform-op-split.mlir
  MLIR :: Dialect/Linalg/transform-tile-reduction.mlir
  MLIR :: Dialect/Tensor/tiling.mlir
  MLIR :: Interfaces/TilingInterface/tile-using-interface.mlir
  MLIR :: Interfaces/TilingInterface/tile-using-scfforall.mlir
  MLIR :: Transforms/parallel-loop-collapsing.mlir
  MLIR :: Transforms/single-parallel-loop-collapsing.mlir


Testing Time: 2.97s

Total Discovered Tests: 3033
  Unsupported      :  515 (16.98%)
  Passed           : 2500 (82.43%)
  Expectedly Failed:    1 (0.03%)
  Failed           :   17 (0.56%)
FAILED: tools/mlir/test/CMakeFiles/check-mlir /root/llvm/llvm-project/build/tools/mlir/test/CMakeFiles/check-mlir 
```


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