Skip to content

[mlir][spirv] Handle vectors of integers of unsupported width #118663

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 4, 2024
Merged

[mlir][spirv] Handle vectors of integers of unsupported width #118663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1968efb
[mlir][spir] Handle vectors of integers of unsupported width
kuhar Dec 4, 2024
1ee3f31
Clarify
kuhar Dec 4, 2024
ce2e778
Typo
kuhar Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,13 +292,19 @@ convertScalarType(const spirv::TargetEnv &targetEnv,
}

/// Converts a sub-byte integer `type` to i32 regardless of target environment.
/// Returns a nullptr for unsupported integer types, including non sub-byte
/// types.
///
/// Note that we don't recognize sub-byte types in `spirv::ScalarType` and use
/// the above given that these sub-byte types are not supported at all in
/// SPIR-V; there are no compute/storage capability for them like other
/// supported integer types.
static Type convertSubByteIntegerType(const SPIRVConversionOptions &options,
IntegerType type) {
if (type.getWidth() > 8) {
LLVM_DEBUG(llvm::dbgs() << "not a subbyte type\n");
return nullptr;
}
if (options.subByteTypeStorage != SPIRVSubByteTypeStorage::Packed) {
LLVM_DEBUG(llvm::dbgs() << "unsupported sub-byte storage kind\n");
return nullptr;
Expand Down Expand Up @@ -348,6 +354,9 @@ convertVectorType(const spirv::TargetEnv &targetEnv,
}

Type elementType = convertSubByteIntegerType(options, intType);
if (!elementType)
return nullptr;
Comment on lines +357 to +358
Copy link
Contributor

@andfau-amd andfau-amd Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug is that previously, it was assumed that if we entered this code path, the integer type was unsupported because of being sub-byte, but it might be unsupported for some other reason?

Copy link
Member Author

@kuhar kuhar Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug was essentially that it assumed that convertSubByteIntegerType would always succeed, but this is not the case even prior to my change related to subbyte types.

Copy link
Contributor

@andfau-amd andfau-amd Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. If you want to, you could put this in the pull-request description so it ends up in the commit message, but that may be unnecessary.


if (type.getRank() <= 1 && type.getNumElements() == 1)
return elementType;

Expand Down
8 changes: 8 additions & 0 deletions mlir/test/Conversion/ArithToSPIRV/arith-to-spirv-unsupported.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,14 @@ func.func @int_vector4_invalid(%arg0: vector<2xi16>) {
return
}

// -----

func.func @int_vector_invalid_bitwidth(%arg0: vector<2xi12>) {
// expected-error @+1 {{failed to legalize operation 'arith.addi'}}
%0 = arith.addi %arg0, %arg0: vector<2xi12>
return
}

///===----------------------------------------------------------------------===//
// Constant ops
//===----------------------------------------------------------------------===//
Expand Down
Loading