Skip to content

[mlir][spirv] Check output of getConstantInt #140568

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.

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
Jun 4, 2025

Conversation

IgWod-IMG
Copy link
Contributor

This patch adds an assert to check if the result of getConstantInt is non-null. Previously the code failed with Segmentation Fault if getConstantInt failed to look up the value. This primarily occurred when the value was defined as OpSpecConstant rather than OpConstant.

This patch adds an assert to check if the result of getConstantInt
is non-null. Previously the code failed with Segmentation Fault if
getConstantInt failed to look up the value. This primarily occurred
when the value was defined as OpSpecConstant rather than OpConstant.
@llvmbot
Copy link
Member

llvmbot commented May 19, 2025

@llvm/pr-subscribers-mlir-spirv

@llvm/pr-subscribers-mlir

Author: Igor Wodiany (IgWod-IMG)

Changes

This patch adds an assert to check if the result of getConstantInt is non-null. Previously the code failed with Segmentation Fault if getConstantInt failed to look up the value. This primarily occurred when the value was defined as OpSpecConstant rather than OpConstant.


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

1 Files Affected:

  • (modified) mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp (+11-4)
diff --git a/mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp b/mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp
index 7afd6e9b25b77..9008d88866f40 100644
--- a/mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp
+++ b/mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp
@@ -1061,12 +1061,19 @@ LogicalResult spirv::Deserializer::processCooperativeMatrixTypeKHR(
            << operands[2];
   }
 
-  unsigned rows = getConstantInt(operands[3]).getInt();
-  unsigned columns = getConstantInt(operands[4]).getInt();
+  IntegerAttr rowsAttr = getConstantInt(operands[3]);
+  assert(rowsAttr);
+  unsigned rows = rowsAttr.getInt();
+
+  IntegerAttr columnsAttr = getConstantInt(operands[4]);
+  assert(columnsAttr);
+  unsigned columns = columnsAttr.getInt();
+
+  IntegerAttr useAttr = getConstantInt(operands[5]);
+  assert(useAttr);
 
   std::optional<spirv::CooperativeMatrixUseKHR> use =
-      spirv::symbolizeCooperativeMatrixUseKHR(
-          getConstantInt(operands[5]).getInt());
+      spirv::symbolizeCooperativeMatrixUseKHR(useAttr.getInt());
   if (!use) {
     return emitError(
                unknownLoc,

Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

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

LGTM % readability

@IgWod-IMG IgWod-IMG force-pushed the img_get-const-int-assert branch from 2526c7a to 19d5056 Compare June 4, 2025 09:43
@IgWod-IMG
Copy link
Contributor Author

I just re-pushed the same commit to re-kick CI. There were some flang failures that looked unrelated, but I just want to double check it.

@IgWod-IMG IgWod-IMG merged commit 3ce3281 into llvm:main Jun 4, 2025
11 checks passed
@IgWod-IMG IgWod-IMG deleted the img_get-const-int-assert branch June 4, 2025 12:15
rorth pushed a commit to rorth/llvm-project that referenced this pull request Jun 11, 2025
This patch adds an assert to check if the result of `getConstantInt` is
non-null. Previously the code failed with Segmentation Fault if
`getConstantInt` failed to look up the value. This primarily occurrs when
the value is defined as OpSpecConstant rather than OpConstant.
DhruvSrivastavaX pushed a commit to DhruvSrivastavaX/lldb-for-aix that referenced this pull request Jun 12, 2025
This patch adds an assert to check if the result of `getConstantInt` is
non-null. Previously the code failed with Segmentation Fault if
`getConstantInt` failed to look up the value. This primarily occurrs when
the value is defined as OpSpecConstant rather than OpConstant.
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.

4 participants