[MLIR][SPIRV] SPIRVConversion.h: bug in getPushConstantValue
- Dominant language
- LLVM
- Stars
- 40.5k
- Forks
- 18.7k
- PR merge metrics
- PR metrics pending
Description
I think I found a bug in the spirv dialect but I am unsure of how to implement a solution. So SPIRVConversion.h has this utility function:
https://github.com/llvm/llvm-project/blob/69902769c74718295dd553a1cd0bb6c56ff2645a/mlir/include/mlir/Dialect/SPIRV/Transforms/SPIRVConversion.h#L162-L169
and the description explicitly states that the push constant has `integerType` integers.
The implementation relies on the assumption that integerType is in fact 32bit. It contains the lines:
https://github.com/llvm/llvm-project/blob/69902769c74718295dd553a1cd0bb6c56ff2645a/mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp#L1323-L1324
Which ends up producing mlir that looks like this (when 64 bit integer is used):
```mlir
%3 = "spirv.Constant"() <{value = 0 : i32}> : () -> i64
```
which causes the pass created by `createSPIRVLowerABIAttributesPass` to fail.
The assumption of integerType being 32bits isn't just used by that one line.
getPushConstantValue implementation also uses functions such as:
https://github.com/llvm/llvm-project/blob/69902769c74718295dd553a1cd0bb6c56ff2645a/mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp#L965-L970
which ends up calling other functions that also assume 32bit integer types, just having a parameter specifying the index type.
Now I don't know if this is a documentation bug. As in `getPushConstantValue` was meant to use `integerType` as an index type like all the other functions, or if `getPushConstantValue` was meant to behave the way its description is written and all the static functions implementing its machinery just assume 32bit `integerType`?
I feel like the most complete solution to this problem would be to have two Type params in getPushConstantValue one for the index type and one for the type the push constant contains. But this would require significantly rewriting its implementation and this function's consumers. And since this function is in a public header, this would be a large and invasive change.
Contributor guide
Assessment
This issue has not been assessed yet.