Skip to content

RTAO: fix denoiser's unsafe wave-lane ↔ SV_GroupIndex ordering assumption - #967

Merged
amarpMSFT merged 1 commit into
microsoft:masterfrom
amarpMSFT:user/amarp/rtao-wave-lane-ordering
Jul 16, 2026
Merged

RTAO: fix denoiser's unsafe wave-lane ↔ SV_GroupIndex ordering assumption#967
amarpMSFT merged 1 commit into
microsoft:masterfrom
amarpMSFT:user/amarp/rtao-wave-lane-ordering

Conversation

@amarpMSFT

@amarpMSFT amarpMSFT commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

What

The RTAO denoiser's CalculateMeanVarianceCS and DisocclusionBlur3x3CS compute shaders exchanged each row's values across wave lanes while deriving source-lane indices from SV_GroupIndex. That assumes row-major thread-to-lane packing and a wave width of at least 16, neither of which HLSL guarantees for these 2D thread groups.

This PR removes that assumption. The affected shaders now exchange values only through groupshared-memory caches indexed by their logical thread coordinates. There is no optional wave path or runtime capability gate.

Why it's a bug

Both live shaders use 2D thread groups ([numthreads(8, 8, 1)]) and previously did the equivalent of:

const uint Row_BaseWaveLaneIndex = (WaveGetLaneIndex() / 16) * 16;
...
float cValue = WaveReadLaneAt(value, Row_BaseWaveLaneIndex + /* column derived from SV_GroupIndex */);

This returns the intended neighbour only if the implementation maps each thread's wave lane index to its row-major SV_GroupIndex. The HLSL SM 6.6 derivatives specification explicitly states that for 2D thread groups "no relation between the values of SV_GroupIndex and the return value of WaveGetLaneIndex() should be assumed." Wave-op support and wave width cannot establish that ordering.

Repro

On WARP (wave width 4), the old shader path reads nonexistent or unrelated lanes. A D3D12 compute harness ran the affected shaders against CPU references:

  • CalculateMeanVariance: the old wave path was wrong on 576/576 tested interior pixels. The groupshared implementation matched to half precision (maximum mean delta 0.000338; maximum variance delta 0.000296).
  • Depth-aware gaussian/disocclusion filtering: the old wave path was wrong on 35/36 tested interior pixels. The groupshared implementation matched the CPU reference.

The fix

  • CalculateMeanVarianceCS.hlsl now loads the logical 16x16 input tile into groupshared memory, synchronizes the group, and calculates each horizontal kernel from that cache.
  • DisocclusionBlur3x3CS.hlsl now does the equivalent groupshared exchange for each (value, depth) pair before applying the depth-aware gaussian weights.
  • The same latent pattern was corrected in the currently-unused depth-aware separable gaussian variant. The file was renamed to DepthAwareSeparableGaussianFilter3x3CS.hlsl because it no longer uses WaveReadLaneAt.
  • Obsolete wave-support requirements and stale measured/relative performance claims were removed from the README and affected shader comments.

Validation

  • All three final shaders compile successfully with dxc as cs_6_3.
  • Their final DXIL is byte-identical to the groupshared binaries exercised by the WARP CPU-reference A/B tests above.
  • A source scan confirms that no WaveReadLaneAt call or deleted fast-path identifier remains in this sample.
  • The final PR adds no C++ or project-system changes; those files are byte-identical to origin/master.
  • git diff --check passes, and the project XML files parse successfully.

Notes for reviewers

The earlier capability-gated wave permutation has been removed rather than retained behind WaveOps/WaveLaneCountMin: those feature values describe intrinsic availability and width, not lane-to-thread ordering. Groupshared exchange is now the sole implementation, so correctness is independent of wave size and lane assignment.

The CalculateMeanVariance and DisocclusionBlur compute shaders exchanged each
row's values across wave lanes while deriving target lane indices from
SV_GroupIndex. For their 2D [numthreads(8,8,1)] groups, HLSL does not guarantee
any relationship between SV_GroupIndex and WaveGetLaneIndex. The shaders also
assumed a wave width of at least 16 without enforcing it. On WARP (wave width
4), the wave reads address nonexistent or unrelated lanes and corrupt output.

Replace those exchanges with groupshared-memory caches indexed by the logical
thread layout. This is independent of wave size and thread-to-lane assignment.
Apply the same correction to the unused depth-aware separable gaussian variant
and rename it now that it no longer uses WaveReadLaneAt.

Remove stale measured and relative performance claims from the README and the
old shader timing comment because they describe the previous implementation.

Validation:
- dxc cs_6_3 compiles all three groupshared shaders.
- Their DXIL is byte-identical to the groupshared binaries used for the WARP
  A/B tests.
- CalculateMeanVariance groupshared output matches a CPU reference to
  half-precision (max mean delta 0.000338, max variance delta 0.000296), while
  the old wave path was wrong on 576/576 tested interior pixels on WARP.
- The depth-aware gaussian groupshared output matches its CPU reference, while
  the old wave path was wrong on 35/36 tested interior pixels on WARP.
@amarpMSFT
amarpMSFT force-pushed the user/amarp/rtao-wave-lane-ordering branch from 5357677 to c09d844 Compare July 16, 2026 03:36

@jenatali Jesse Natalie (jenatali) 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.

LGTM

@amarpMSFT
amarpMSFT merged commit 357ade6 into microsoft:master Jul 16, 2026
1 check passed
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.

2 participants