[Mlir-commits] [mlir] [mlir][vector] Allow signless integer element types in `vector.step` (PR #205142)

[Andrzej Warzyński]

banach-space wrote:

OK, this is a bit more subtle than i thought. 

I am not against relaxing the return type in `vector.step`, but I am not too fond of encoding too much subtle semantics into the Op lowering (e.g. wraparound semantics). I feel that we might be over-iterating for edge cases. 

As a I concrete example, let me take this test:
```mlir
func.func @step_i8_truncate() -> vector<258xi8> {
  %0 = vector.step : vector<258xi8>
  return %0 : vector<258xi8>
}
```

The wraparound semantics will only matter if `vector.step` is lowered to `arith.constant`. However, with 258 elements (~60k for `i16`), is it really desirable to use `arith.constant` instead? In all of my cases I would prefer to lower to `llvm.stepvector` instead. That's already the default behaviour for scalable vectors: https://github.com/llvm/llvm-project/blob/0b5c006208cf7b0fee3709f468152e7de81607ef/mlir/lib/Dialect/Vector/Transforms/LowerVectorStep.cpp?plain=1#L32-L34

(`StepToArithConstantOpRewrite` bails out for scalable vectors).

>  I'm happy to do a proper fix, passing the index bitwidth into the pattern as an argument (defaulting to 64 for backward compatibility)

Alternatively, we could make `StepToArithConstantOpRewrite` bail out for large vectors (i.e. > 256 elements) )and that would be my preference. @kuhar , does _anything_ depend on `StepToArithConstantOpRewrite`? Would restricting it be OK? 

@victor-eds , you could also check whether https://mlir.llvm.org/docs/DataLayout/?utm_source=chatgpt.com#index-type can be leveraged for testing, but IMO it would only make sense if you could set the index width to 8 (to avoid printing very long vectors of numbers).

I will also post some comments inline.

Oh, and thanks for working on this, all in all looks sensible and is a clear improvement!

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