Skip to content

fix: emit dimension-correct HLSL texture sample offsets - #9951

Merged
ErichDonGubler merged 4 commits into
gfx-rs:trunkfrom
mvanhorn:fix/9717-invalid-hlsl-for-texturesamplelevel-of-3d-textur
Jul 30, 2026
Merged

fix: emit dimension-correct HLSL texture sample offsets#9951
ErichDonGubler merged 4 commits into
gfx-rs:trunkfrom
mvanhorn:fix/9717-invalid-hlsl-for-texturesamplelevel-of-3d-textur

Conversation

@mvanhorn

@mvanhorn mvanhorn commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Connections

Summary

The HLSL backend wrapped every image-sampling offset in int2(...) as a DXC overload-resolution workaround. For a textureSampleLevel call on other-dimensioned sampled textures, that's the wrong type. For example, with 3D textures, the writer emitted a nested int2(int3(...)) expression, which fails to compile.

This makes the constructor follow the offset expression's own dimension, emitting int, int2, or int3 as appropriate, while keeping the explicit-constructor workaround DXC needs.

Testing

  • Added a wgsl-9717-texture-sample-level-offset.hlsl snapshot test that emits checks for all dimensions of texture with textureSampleLevel.

Squash or Rebase?

Rebase.

Checklist

  • I self-reviewed and fully understand this PR.
  • WebGPU implementations built with wgpu may be affected behaviorally.
  • Tests demonstrate the validation and altered logic works.
  • CHANGELOG.md entries for the user-facing effects of this change are present.
  • The PR is minimal, and doesn't make sense to land as multiple PRs.
  • Commits are logically scoped and individually reviewable.
  • The PR description has enough context to understand the motivation and solution implemented.

@ErichDonGubler ErichDonGubler self-assigned this Jul 28, 2026

@ErichDonGubler ErichDonGubler left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks like a good start, and I like the snapshot test. There are a few things that still need to happen, though, before I'd approve this:

  • Handle the feedback I left as individual conversations in the overall diff.
  • The OP is verbose to the point of hurting communication (LLM output?), so I edited it down to the useful bits.
  • The issue was motivated by CTS tests, and we should prove we're solving it with the same. Please add some CTS test coverage, preferably for all dimensionalities.

LMK if you need anything else!

Comment thread naga/src/back/hlsl/writer.rs Outdated
Comment thread naga/src/back/hlsl/writer.rs Outdated
Comment thread naga/src/back/hlsl/writer.rs Outdated
Comment thread CHANGELOG.md Outdated
@ErichDonGubler ErichDonGubler added type: bug Something isn't working backend: dx12 Issues with DX12 or DXGI area: naga back-end Outputs of naga shader conversion naga Shader Translator lang: HLSL D3D Shading Language labels Jul 28, 2026
Addresses @ErichDonGubler's review.

writer.rs: use TypeInner::vector_size_and_scalar instead of re-implementing
the match, switch debug_assert_eq! to assert_eq!, and hoist the DXC issue
comment above the workaround block where it explains the whole thing.

CHANGELOG.md: take the suggested wording, imperative and scoped to all
non-2D textures rather than just 3D.

cts_runner/test.lst: add the 1D and 3D textureSampleLevel execution
selectors, the non-2D dimensionalities whose offset constructor was wrong.
Verified locally: sampled_3d_coords 3357 pass / 0 fail, sampled_1d_coords
972 pass / 0 fail. Those ran on Metal, so CI's Windows runner is what
actually exercises the HLSL backend this fix touches.
@mvanhorn

Copy link
Copy Markdown
Contributor Author

384870e covers all of it.

The three inline notes: vector_size_and_scalar() replaces the hand-rolled match, debug_assert_eq! is now assert_eq!, and the DXC issue comment is hoisted above the block it explains. CHANGELOG takes your wording, scoped to non-2D rather than just 3D.

On CTS: added the sampled_1d_coords and sampled_3d_coords execution selectors to cts_runner/test.lst, which are the non-2D dimensionalities whose offset constructor was wrong. Locally sampled_3d_coords is 3357 pass / 0 fail and sampled_1d_coords is 972 pass / 0 fail.

Worth being precise about what that does and does not show: I am on macOS, so those ran on Metal and cannot exercise the HLSL backend this fix touches. The CI matrix includes Windows x86_64, so landing these selectors is what actually puts the fix under CTS on dx12. The naga snapshot test remains the direct check on the generated HLSL.

One thing I found while there and did not touch: sampled_array_2d_coords fails locally on Metal for every case including offset=false, at depthOrArrayLayers=8 while 1 passes. It is unrelated to this change, so I left it out of test.lst rather than adding a known-failing selector. Happy to file it separately if that is useful.

Full naga suite: 216 pass, 0 fail, with clippy clean.

@ErichDonGubler

Copy link
Copy Markdown
Member

Okay, this is looking good. However, I just noticed another problem that means we can't merge the current test coverage. 🫠 CTS test times for Other on Windows exploded, going from ~20m to ~3 hours.

tenor

We need to fix this, but I don't want to hold up this fix for that. Let's punt that to another issue/PR where we figure that out: #9974

CC @gfx-rs/wgpu ☝🏻

@ErichDonGubler ErichDonGubler left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving because of removed CTS coverage. Sigh. 🫠

@ErichDonGubler
ErichDonGubler enabled auto-merge (squash) July 30, 2026 11:31
@ErichDonGubler
ErichDonGubler merged commit c079e55 into gfx-rs:trunk Jul 30, 2026
60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: naga back-end Outputs of naga shader conversion backend: dx12 Issues with DX12 or DXGI lang: HLSL D3D Shading Language naga Shader Translator type: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Invalid HLSL for textureSampleLevel of 3d texture with offset

2 participants