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

ConvertFloat32ToFloat16: Use DirectXMath conversion functions #4855

Merged
merged 9 commits into from
Dec 15, 2022

Conversation

tex3d
Copy link
Contributor

@tex3d tex3d commented Dec 7, 2022

Custom half <-> float conversion functions had problems in multiple scenarios. This PR changes them into a wrapper, using the DirectXMath conversion functions instead.

@AppVeyorBot
Copy link

Updated to OR in the float32 fractional bits, since truncation is the implied rounding mode for this function.
Updated the name to better reflect the meaning.
@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

Implementation of conversion stubs that pass through to DirectXMath
functions is in ExecutionTest.cpp, since all uses of these conversions
are within this cpp or ShaderOpTest.cpp which is in the same lib.

This location only depends on DirectXMath where we expect win32-only build.
@AppVeyorBot
Copy link

@tex3d tex3d requested a review from hekota December 14, 2022 02:38
@tex3d tex3d marked this pull request as ready for review December 14, 2022 02:38
@tex3d tex3d changed the title ConvertFloat32ToFloat16: convert value larger than max f16 to INF ConvertFloat32ToFloat16: Use DirectXMath conversion functions Dec 14, 2022
@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

float ConvertFloat16ToFloat32(uint16_t Value) throw() {
return DirectX::PackedVector::XMConvertHalfToFloat(Value);
}

Copy link
Member

Choose a reason for hiding this comment

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

I know you want Helena to comment here and she should, but we talked about this earlier and she plans to move a lot of the ShaderOp code into its own directory. That might resolve any concerns about dependencies involving execution tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fortunately, this cpp file is already dependent on DirectXMath. So, I don't think there will be any trouble with this change. I still wanted to double check that it doesn't break tests in some unexpected way though.

Copy link
Member

Choose a reason for hiding this comment

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

I believe this should be fine, as Tex said the dependency is already there, and these headers should be available in the Windows build. It is the LLVM dependencies and additional DXC headers that are problematic.

Copy link
Member

@hekota hekota left a comment

Choose a reason for hiding this comment

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

LGTM!

float ConvertFloat16ToFloat32(uint16_t Value) throw() {
return DirectX::PackedVector::XMConvertHalfToFloat(Value);
}

Copy link
Member

Choose a reason for hiding this comment

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

I believe this should be fine, as Tex said the dependency is already there, and these headers should be available in the Windows build. It is the LLVM dependencies and additional DXC headers that are problematic.

@tex3d tex3d merged commit 6acd11b into microsoft:main Dec 15, 2022
@tex3d tex3d deleted the conv-f32-f16-gt-max-to-inf branch December 15, 2022 01:40
@tex3d tex3d restored the conv-f32-f16-gt-max-to-inf branch December 15, 2022 01:40
hekota pushed a commit to hekota/DirectXShaderCompiler that referenced this pull request Dec 16, 2022
…oft#4855)

Custom half <-> float conversion functions had problems in multiple scenarios.  This PR changes them into a wrapper, using the DirectXMath conversion functions instead.

(cherry picked from commit 6acd11b)
hekota added a commit that referenced this pull request Dec 16, 2022
Cherry-pick of PIX and HLK changes for the 2212 release

Changes for PIX:
20bb3d0 PIX: Modify root sigs in place (plus fix root sig memory leak) (PIX: Modify root sigs in place (plus fix root sig memory leak) #4876)
2c3d965 dxcopt: Support full container and restore extra data to module (dxcopt: Support full container and restore extra data to module #4845)
21cf36a Fix hitgroup metadata argument order

HLK Test Updates:
ee0994e add barycentrics ordering check onto existing barycentrics test (add barycentrics ordering check onto existing barycentrics test #4635)
6acd11b ConvertFloat32ToFloat16: Use DirectXMath conversion functions (ConvertFloat32ToFloat16: Use DirectXMath conversion functions #4855)
e7aac8e Include TestConfig.h only if DEFAULT_TEST_DIR is not defined (Include TestConfig.h only if DEFAULT_TEST_DIR is not already defined #4884)
5decc4a Do not include TestConfig.h for all HLK build (Do not include TestConfig.h for all HLK build #4887)
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