Skip to content

Support casting of OpConstantNull to SPIRVConstant#3238

Merged
MrSidims merged 2 commits intoKhronosGroup:mainfrom
vmaksimo:constant-null
Jun 18, 2025
Merged

Support casting of OpConstantNull to SPIRVConstant#3238
MrSidims merged 2 commits intoKhronosGroup:mainfrom
vmaksimo:constant-null

Conversation

@vmaksimo
Copy link
Contributor

Current design does not imply inheritance between SPIRVConstantNull and SPIRVConstant classes, however it is possible that we do the static_cast of SPIRVValue to SPIRVConstant for null constant. Return zero-value in that case.

Current design does not imply inheritance between `SPIRVConstantNull` and `SPIRVConstant` classes,
however it is possible that we do the static_cast of `SPIRVValue` to
`SPIRVConstant` for null constant. Return zero-value in that case.
@vmaksimo vmaksimo requested a review from MrSidims June 16, 2025 13:02
@vmaksimo
Copy link
Contributor Author

vmaksimo commented Jun 16, 2025

Without the fix the test case fails on the validation of matrix type here: https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/lib/SPIRV/libSPIRV/SPIRVType.cpp#L375
And without validation it does not fail, but translates zero as 43793728 here: https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/lib/SPIRV/libSPIRV/SPIRVType.cpp#L375

Comment on lines +183 to +184
if (OpCode == OpConstantNull)
return 0;
Copy link

@Naghasan Naghasan Jun 16, 2025

Choose a reason for hiding this comment

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

nit

Suggested change
if (OpCode == OpConstantNull)
return 0;
if (OpCode == OpConstantNull) {
assert(std::is_arithmetic_v<T> && "OpConstantNull has no defined value for this type");
return 0;
}

Copy link
Contributor

@MrSidims MrSidims Jun 16, 2025

Choose a reason for hiding this comment

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

I actually have another Q. In which contexts getValue is called? Can it be called to get value of an object of Event type or a Coop Matrix or any composite? If yes note, that ConstantNull is defined as:

Result Type must be one of the following types:
- Scalar or vector Boolean type
- Scalar or vector integer type
- Scalar or vector floating-point type
- Pointer type
- Event type
- Device side event type
- Reservation id type
- Queue type
- Composite type

hence here we should:
a. not add the proposed assetion;
b. return not null, but a value constructed by the T's default c'tor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I got the answer: the method is protected and used only by:

  uint64_t getZExtIntValue() const { return getValue<uint64_t>(); }
  float getFloatValue() const { return getValue<float>(); }
  double getDoubleValue() const { return getValue<double>(); }

hence we can even make the proposed by Viktor assert to be a static_assert

Choose a reason for hiding this comment

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

Actually, I got the answer: the method is protected and used only by:

yeah, that's why I suggested the nit, I suggested an assert because the function is all constants not just null constant.

@MrSidims MrSidims merged commit dcfd949 into KhronosGroup:main Jun 18, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants