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

Clarify behavior of (norm) packing built-in functions #4159

Closed
teoxoy opened this issue Jun 1, 2023 · 6 comments
Closed

Clarify behavior of (norm) packing built-in functions #4159

teoxoy opened this issue Jun 1, 2023 · 6 comments
Labels
wgsl WebGPU Shading Language Issues

Comments

@teoxoy
Copy link
Member

teoxoy commented Jun 1, 2023

According to the comment #1189 (comment) on the PR that initially added those to the spec, they are supposed to map to operations in SPIR-V's GLSL.std.450 extended instruction set, MSL built-in functions and only need to be polyfilled on HLSL.

However, there is discrepancy between the comment and what got landed as part of the PR.

The behavior of the SPIR-V operations and the HLSL polyfill in the comment use round(x) instead of floor(0.5 + x) as dictated by the contents of the PR (and current version of the spec).

For SPIR-V, the definition of round one should use is not explicitly stated in the sections of each of the packing functions but I would assume the Round op in the same file is what should be used? If so, it states that:

Result is the value equal to the nearest whole number to x. The fraction 0.5 rounds in a direction chosen by the implementation, presumably the direction that is fastest. This includes the possibility that Round x is the same value as RoundEven x for all values of x.

which sounds like bad news since it's implementation defined.

from https://registry.khronos.org/SPIR-V/specs/unified1/GLSL.std.450.html

The behavior of HLSL's round intrinsic is:

Rounds the specified value to the nearest integer. Halfway cases are rounded to the nearest even.

which sounds the same as SPIR-V's RoundEven.

from https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-round

Regarding MSL, its spec doesn't say how the f32 -> norm16 conversion happens. We can of course test the behavior on the range of hardware we are targeting but it would be nice to have a guarantee for a specific behavior.

Questions

  • Was the use of floor intended? If so, it sounds like only MSL could? have a fast path for these functions.
  • If not, are we ok with SPIR-V's round behavior being implementation defined? Or is it always RoundEven in practice (and how do we make sure)?
  • Either way I think we need some clarity on MSL's behavior here. Can we get that? cc @litherum
@teoxoy teoxoy added the wgsl WebGPU Shading Language Issues label Jun 1, 2023
@dneto0
Copy link
Contributor

dneto0 commented Jun 6, 2023

The rounding mode is not specified. It may round up or down.

The conformance tests are written to allow either rounding.
The test code bottoms out at https://github.com/gpuweb/cts/blob/main/src/webgpu/util/math.ts#L527 where it produces all the permitted values. If x falls between two representable f16 values, then both straddling values are allowed.

@Elabajaba
Copy link

Elabajaba commented Jun 7, 2023

Hi, I'm the person that ran into this while implementing HLSL polyfills for the bitpacking/unpacking ops for naga/wgpu.

The pack2x16snorm and pack4x8snorm tests only include a single result, but in my testing the polyfills from #1189 (comment) give different results for any of the -0.5 tests.

edit: eg. for pack4x8snorm([-0.5, -0.5, -0.5, -0.5]) the polyfill gives 0xc0c0c0c0 while the expected result according to the CTS tests is 0xc1c1c1c1

@kdashg
Copy link
Contributor

kdashg commented Jun 7, 2023

WGSL 2023-06-06 Minutes
  • DN: As with all rounding for FP, it can round up or down. Will look at making that optionality clearer in the spec. And CTS is written to allow either behaviour.

@magcius
Copy link

magcius commented Jun 7, 2023

It seems like there are two tests for pack4x8snorm in the CTS. There's one test in webgpu/execution/expression/call/builtin/pack4x8snorm which is properly written to respect rounding mode, and there's a separate test in unittests/conversion, which assumes a certain rounding mode.

Should the tests in unittests/conversion.spec.ts be ignored?

@kdashg
Copy link
Contributor

kdashg commented Jun 14, 2023

I believe this is just a test bug then!

@kdashg kdashg closed this as completed Jun 14, 2023
@teoxoy
Copy link
Member Author

teoxoy commented Jun 14, 2023

The rounding mode is not specified. It may round up or down.

We seem to have floor(0.5 + x) in the spec right now instead of round(x) (where rounding mode is not specified).

image

I think this should be editorial to fix the text in the spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wgsl WebGPU Shading Language Issues
Projects
None yet
Development

No branches or pull requests

5 participants