Skip to content

Conversation

@kuhar
Copy link
Member

@kuhar kuhar commented Dec 4, 2024

Fixes: #118612

@kuhar kuhar requested a review from antiagainst as a code owner December 4, 2024 16:18
@kuhar kuhar changed the title [mlir][spir] Handle vectors of integers of unsupported width [mlir][spirv] Handle vectors of integers of unsupported width Dec 4, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 4, 2024

@llvm/pr-subscribers-mlir-spirv

@llvm/pr-subscribers-mlir

Author: Jakub Kuderski (kuhar)

Changes

Fixes: #118612


Full diff: https://github.com/llvm/llvm-project/pull/118663.diff

2 Files Affected:

  • (modified) mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp (+7)
  • (modified) mlir/test/Conversion/ArithToSPIRV/arith-to-spirv-unsupported.mlir (+8)
diff --git a/mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp b/mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp
index f5700059f68ee2..d8b67bb4389ea2 100644
--- a/mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp
+++ b/mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp
@@ -299,6 +299,10 @@ convertScalarType(const spirv::TargetEnv &targetEnv,
 /// 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;
@@ -348,6 +352,9 @@ convertVectorType(const spirv::TargetEnv &targetEnv,
     }
 
     Type elementType = convertSubByteIntegerType(options, intType);
+    if (!elementType)
+      return nullptr;
+
     if (type.getRank() <= 1 && type.getNumElements() == 1)
       return elementType;
 
diff --git a/mlir/test/Conversion/ArithToSPIRV/arith-to-spirv-unsupported.mlir b/mlir/test/Conversion/ArithToSPIRV/arith-to-spirv-unsupported.mlir
index 24a0bab352c345..9d7ab2be096ef7 100644
--- a/mlir/test/Conversion/ArithToSPIRV/arith-to-spirv-unsupported.mlir
+++ b/mlir/test/Conversion/ArithToSPIRV/arith-to-spirv-unsupported.mlir
@@ -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
 //===----------------------------------------------------------------------===//

@kuhar kuhar requested a review from andfau-amd December 4, 2024 16:28
Comment on lines +356 to +357
if (!elementType)
return nullptr;
Copy link
Contributor

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

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

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.

@kuhar kuhar requested a review from andfau-amd December 4, 2024 16:37
@kuhar kuhar merged commit bb9bb68 into llvm:main Dec 4, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[mlir][spirv] -convert-to-spirv crashes

3 participants