llvm / llvm/llvm-project

[MLIR][SPIRV] SPIRVConversion.h: bug in getPushConstantValue

Open
#185,149 7 comments 0 reactions 1 assignee Claimed by @efric View on GitHub
mlir:spirv
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.