Skip to content
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

Support Narrow Types in BitCast Folding Rule #4941

Merged
merged 3 commits into from
Oct 6, 2022

Conversation

gmitrano-unity
Copy link
Contributor

This change adds support for narrow types in the BitCastScalarOrVector folding rule. According to Section 2.2.1 of the SPIR-V spec, types that are narrower than 32 bits are automatically either sign extended, or zero extended depending on the type. With that guaranteed, we should be able to use the first 32-bit word of any narrow type for the folding logic without performing any special conversions.

In order to reduce code duplication, this change moves the GetU32BitValue and GetU64BitValue functions from IntConstant to ScalarConstant. Without this move, we would have needed an identical version of GetU32BitValue on FloatConstant.

This change adds support for narrow types in the BitCastScalarOrVector
folding rule. According to Section 2.2.1 of the SPIR-V spec, types that
are narrower than 32 bits are automatically either sign extended, or
zero extended depending on the type. With that guaranteed, we should
be able to use the first 32-bit word of any narrow type for the folding
logic without performing any special conversions.

In order to reduce code duplication, this change moves the
GetU32BitValue and GetU64BitValue functions from IntConstant to
ScalarConstant. Without this move, we would have needed an identical
version of GetU32BitValue on FloatConstant.
@CLAassistant
Copy link

CLAassistant commented Sep 21, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

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

The implementation is good.

But this change should come with a unit test.
See test/opt/fold_test.cpp

@@ -163,6 +163,21 @@ class ScalarConstant : public Constant {
return is_zero;
}

uint32_t GetU32BitValue() const {
// Relies on unsigned values smaller than 32-bit being zero extended. See
// section 2.2.1 of the SPIR-V spec.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've confirmed that the SPIR-V validator checks this rule in the LIteralsPass
(Phew!)

error: line 9: The high-order bits of a literal number in instruction 9 must be 0 for a floating-point type, or 0 for an integer type with Signedness of 0, or sign extended when Signedness is 1
%half_n0x1_bfcp_16 = OpConstant %half -0x1.bfcp+16

This change adds several new test cases to the
IntegerInstructionFoldingTest which trigger the 16-bit BitCast logic.
The logic for half types was also added to the integer case since we
can't easily validate half float types in C++ code. It's easier to
validate them as unsigned integers instead. Pllus this also allows us
to verify the SPIR-V constant sign extension logic too.
@gmitrano-unity
Copy link
Contributor Author

Just pushed up some new test cases in the IntegerInstructionFoldingTest suite that cover 16-bit type casting. I handled the 16-bit floats in there too because I didn't see a clean way to integrate them into the FloatInstructionFoldingTest since it uses 32-bit C++ floats as the comparison value. I would have had to do some ugly casting to make them work there, so I figured it made more sense to treat them as integers. (Plus it also allows the 32-bit sign/zero extension logic to be tested)

This change adds a couple more test cases to the integer instruction
folding test suite in order to ensure that the BitCast logic also
works correctly with the Int8 shader capability.
@gmitrano-unity
Copy link
Contributor Author

Just added two more test cases to help get some coverage for 8-bit constants.
Please let me know if anything else is missing! 🙂

@gmitrano-unity
Copy link
Contributor Author

The implementation is good.

But this change should come with a unit test. See test/opt/fold_test.cpp

Hi @dneto0, just wanted to follow up on this and see if the tests I added were sufficient or if I need to add more?

Copy link
Collaborator

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

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

Thanks for the test cases and for your patience in waiting for the review.

@gmitrano-unity
Copy link
Contributor Author

Thanks for the test cases and for your patience in waiting for the review.

No problem! Thank you for the review! 🙂

@dneto0 dneto0 merged commit 1cecf91 into KhronosGroup:master Oct 6, 2022
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.

None yet

4 participants