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

Add data packing builtin functions #1189

Merged
merged 4 commits into from
Nov 10, 2020
Merged

Add data packing builtin functions #1189

merged 4 commits into from
Nov 10, 2020

Conversation

dneto0
Copy link
Contributor

@dneto0 dneto0 commented Oct 27, 2020

  • Add placeholder for unpacking builtin functions
  • Expand the description of floating point conversions now that
    we have to handle edge cases for f32 -> f16.
    I put in the behaviour of an NVIDIA GPU for the concerning edge
    case, but added an Issue to review this for portability.

@dneto0 dneto0 requested review from dj2, kvark and litherum October 27, 2020 22:44
@dneto0
Copy link
Contributor Author

dneto0 commented Oct 27, 2020

Partly addresses #988

@dneto0
Copy link
Contributor Author

dneto0 commented Oct 27, 2020

@dneto0 dneto0 added for wgsl meeting wgsl WebGPU Shading Language Issues labels Oct 27, 2020
wgsl/index.bs Outdated Show resolved Hide resolved
@@ -3267,6 +3272,9 @@ This kind of collision occurs for pairs of adjacent integers with a magnitude of

Issue: (dneto) Default rounding mode is an implementation choice. Is that what we want?

Issue: Check behaviour of the f32 to f16 conversion for numbers just beyond the max normal f16 values.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I just saw SwiftShader convert a finite f32 value larger than the largest f16 finite value into a f16 NaN. That's not permitted in the Vulkan spec, but let me verify my findings first.

* Add placeholder for unpacking builtin functions
* Expand the description of floating point conversions now that
  we have to handle edge cases for f32 -> f16.
  I put in the behaviour of an NVIDIA GPU for the concerning edge
  case, but added an Issue to review this for portability.
Make this consistent with the proposed channel format names
@dneto0
Copy link
Contributor Author

dneto0 commented Oct 30, 2020

Rebased and addressed @dj2's comment.

The main issue is the addition of general language about converting on floating point type value to another floating point type value. The interesting case if f32 to f16 as it introduces cases where finite source numbers should map to either infinite f16 or max normal f16 values. It's not clear what the dividing line is. I've added an "Issue" to verify behaviour and revisit in the future.

@dneto0 dneto0 mentioned this pull request Oct 30, 2020
@dneto0 dneto0 added this to the MVP milestone Oct 30, 2020
@dneto0 dneto0 requested a review from dj2 October 30, 2020 20:27
@grorg grorg added this to Open PRs in WGSL Nov 3, 2020
Copy link
Contributor

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Do we have an investigation somewhere showing how these map to shading languages?

wgsl/index.bs Outdated Show resolved Hide resolved
@grorg
Copy link
Contributor

grorg commented Nov 3, 2020

Discussed at 2020-11-03 meeting.

@grorg
Copy link
Contributor

grorg commented Nov 3, 2020

Resolved: Accept PR (although @kvark mentioned a small error)

@dneto0
Copy link
Contributor Author

dneto0 commented Nov 3, 2020

UGH. I made a mistake claiming these were already present 1-1 in all the backends. I had checked Metal but skimped on HLSL. (!)

Do we have an investigation somewhere showing how these map to shading languages?

I'll add details here.

@dneto0
Copy link
Contributor Author

dneto0 commented Nov 3, 2020

  • WGSL pack2x16float(v : vec2<f32>) -> u32

    • SPIR-V GLSL.std.450 extended instruction PackHalf2x16
    • MSL: as_type<uint>(packed_half2(v.x, v.y))
    • HLSL: f32tof16(f.x) | (f32tof16(f.y) << 16)
  • WGSL: pack4x8snorm(e: vec4<f32>)

    • SPIR-V GLSL.std.450 extended instruction PackSnorm4x8
    • MSL: pack_float_to_snorm4x8(v)
    • HLSL: uint4 temp = round(clamp(v, -1.0f, 1.0f)*127.0); return (temp.x & 0xff) | ((temp.y&0xff) <<8) | ((temp.z&0xff) <<16) | ((temp.w & 0xff) <<24);
  • WGSL: pack4x8unorm(e: vec4<f32>)

    • SPIR-V GLSL.std.450 extended instruction PackUnorm4x8
    • MSL: pack_float_to_unorm4x8(v)
    • HLSL: uint4 temp = round(clamp(v, 0.0f, 1.0f)*255.0); return (temp.x & 0xff) | ((temp.y&0xff) <<8) | ((temp.z&0xff) <<16) | ((temp.w & 0xff) <<24);
  • WGSL: pack2x16snorm(e: vec4<f32>)

    • SPIR-V GLSL.std.450 extended instruction PackSnorm2x16
    • MSL: pack_float_to_snorm2x16(v)
    • HLSL: uint4 temp = round(clamp(v, -1.0f, 1.0f)*32767.0); return (temp.x & 0xffff) | ((temp.y&0xffff) <<16);
  • WGSL: pack2x16unorm(e: vec4<f32>)

    • SPIR-V GLSL.std.450 extended instruction PackUnorm2x16
    • MSL: pack_float_to_unorm2x16(v)
    • HLSL: uint4 temp = round(clamp(v, 0.0f, 1.0f)*65535.0); return (temp.x & 0xffff) | ((temp.y&0xffff) <<16);
  • WGSL: pack2x16unorm(e: vec4<f32>)

    • SPIR-V GLSL.std.450 extended instruction PackUnorm2x16
    • MSL: pack_float_to_unorm2x16(v)
    • HLSL: uint4 temp = round(clamp(v, 0.0f, 1.0f)*65535.0); return (temp.x & 0xffff) | ((temp.y&0xffff) <<16);

@dneto0
Copy link
Contributor Author

dneto0 commented Nov 3, 2020

I filed #1201 to track the edge case behaviour of conversion from f32 to f16

@grorg
Copy link
Contributor

grorg commented Nov 10, 2020

Discussed at the 2020-11-10 meeting.

Resolved: Accept with @dneto0 's changes.

@dneto0
Copy link
Contributor Author

dneto0 commented Nov 10, 2020

Resolved: Accept with @dneto0 's changes.

The "changes" are really implementor's notes. The behaviour is already fully specified in the WGSL spect text in the PR.

@dneto0 dneto0 merged commit 04ab2c9 into gpuweb:main Nov 10, 2020
WGSL automation moved this from Open PRs to Done Nov 10, 2020
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
The current interpolation tests will not attempt to validate an
interpolation with just the type (e.g. `@interpolate(flat)`). This is
due to the `,` always being appended so `@interpolate(flat, )` is
generated which is not a valid value.

This PR updates the generation to only add the `,` if there is a
sampling value to be appended. 

Issue: gpuweb#1135
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
WGSL
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants