Implement Pack/Unpack for HLSL

[Elabajaba]

These are currently untested, and I'm not sure how to do the signed versions.

edit: These have now all been tested against a re-implementation of the CTS tests here (set WGPU_BACKEND env variable to dx12 and run the naga-valid example, it'll print out any failed tests).

Status:

• extractBits
• insertBits
• pack4x8snorm
• pack4x8unorm
• pack2x16snorm
• pack2x16unorm
• pack2x16float
• unpack4x8snorm
• unpack4x8unorm
• unpack2x16snorm
• unpack2x16unorm
• unpack2x16float (was already implemented)

Related: #1449
Closes #1929

[Elabajaba]

The HLSL output for what I've implemented is now valid (I converted a wgsl file containing all the functions to hlsl, then copied that into https://shader-playground.timjones.io/ to make sure that it'd compile with DXC and FXC), but I'm not sure how to test that the output is correct.

[teoxoy]

Please add HLSL here

naga/tests/snapshots.rs

Line 448 in 907b7c7

Targets::SPIRV | Targets::METAL | Targets::GLSL | Targets::WGSL,

so that we have and check the HLSL snapshots. CI will only make sure the output is valid HLSL but not run the file.

but I'm not sure how to test that the output is correct.

We aren't currently testing behavior in naga, what I'd do is create a compute shader to test the behavior and open a PR on wgpu (that temporarily links to your rev of naga).

[Elabajaba]

I hacked up a compute shader to test with (though I'm currently running it as an example and not a test, see the naga-valid example https://github.com/Elabajaba/wgpu/tree/mynagatest).

I've run into an issue where my pack4x8snorm and pack2x16snorm implementations are off by 1 compared to the spec when given -0.5 (for pack2x16snorm([-0.5, -0.5]), spec: 0xc001c001, mine: 0xc000c000), but I get the same off by 1 result when running it with Vulkan (Windows + AMD).

packing spec: https://github.com/gpuweb/cts/blob/main/src/unittests/conversion.spec.ts
unpacking spec: https://github.com/gpuweb/cts/blob/main/src/unittests/floating_point.spec.ts

[teoxoy]

The issue is that the WGSL spec defines the behavior of pack2x16snorm as follows:

Component e[i] of the input is converted to a 16-bit twos complement integer value ⌊ 0.5 + 32767 × min(1, max(-1, e[i])) ⌋ which is then placed in bits 16 × i through 16 × i + 15 of the result.

We need to floor instead of round which is the behavior of SPV and what the PR is currently implementing.

Having to polyfill this for SPV as well is unfortunate but I guess necessary since the rounding behavior might not always be the same across implementations of SPV.

The fraction 0.5 rounds in a direction chosen by the implementation, presumably the direction that is fastest.

[teoxoy]

I wonder how MSL behaves since they don't specify this in their spec.

[teoxoy]

I filed gpuweb/gpuweb#4159 to hopefully iron out the behavior. Maybe we can wait a bit and see how to proceed.

[Elabajaba]

I could've saved so much time had I known about gpuweb/gpuweb#1189 (comment)...

[teoxoy]

I feel you 😅 - the spec repo usually contains a lot of info in PRs/issues since the decisions are taken based on what's in the APIs/shading languages it targets. I find myself using git log -S on the spec repo quite often.

[Elabajaba]

insertBits is passing every test except the 32 count tests, and I'm not sure how to fix it.

insertBits(e: 0, newbits: 0b11111111111111111111111111111111, offset: 0, count: 32)
result:   0b00000000000000000000000000000000,
expected: 0b11111111111111111111111111111111

readable version of what I'm doing:

    let mask = ((((1u << count) - 1u) << offset) & 0xffffffffu);
    return = ((newbits << offset) & mask) | (e & ~mask);

[magcius]

Could you do let mask = (0xFFFFFFFFu >> (32 - count)) << offset instead?

[Elabajaba]

Could you do let mask = (0xFFFFFFFFu >> (32 - count)) << offset instead?

Fails the count = 0 tests :(

edit: Added a ternary to guard against count == 0, now it works.

[Elabajaba]

The spec also calls for us to error on insertbits if we try and insert too much https://www.w3.org/TR/WGSL/#insertBits-builtin

If count + offset is greater than w (w is the bitwidth of T, where T is i32, u32, vecN<i32>, or vecN<u32>), then:

It is a shader-creation error if count and offset are const-expressions.

It is a pipeline-creation error if count and offset are override-expressions.

We don't currently do that for HLSL or Vulkan, and I don't know how to implement that.

[teoxoy]

We can take care of that validation later once we got const-expressions fully working.

[teoxoy]

I have not yet looked at the behavior but still left some comments.

Also, it seems the resolution of gpuweb/gpuweb#4159 is that the rounding mode should have been unspecified so, we can use round (same as here gpuweb/gpuweb#1189 (comment)) instead of floor(0.5 + x) for the packing functions.

[teoxoy]

Looks great! Left a few minor comments.

[teoxoy]

I wanted to point out that for insertBits and extractBits we are not capping the offset and count.
Considering the MSL and SPIR-V backends also require this it might be better for the WGSL frontend to inject those.

We'll also end up with a lot of duplication if we want to do it in-line as opposed to calling a function we injected previously (this is how we deal with constructors currently).

[teoxoy]

Note for a future reader (including myself): HLSL seems to use arithmetic right shifts for signed integers even if not specified in their docs - found this out by looking at the output of FXC and DXC.

[teoxoy]

Thank you for implementing this, it looks great!✨