Skip to content

Clean up Wave preprocessor handling#1022

Merged
AnastaZIuk merged 6 commits intoDevsh-Graphics-Programming:masterfrom
AnastaZIuk:nscFixes
Mar 18, 2026
Merged

Clean up Wave preprocessor handling#1022
AnastaZIuk merged 6 commits intoDevsh-Graphics-Programming:masterfrom
AnastaZIuk:nscFixes

Conversation

@AnastaZIuk
Copy link
Copy Markdown
Member

@AnastaZIuk AnastaZIuk commented Mar 18, 2026

Summary

This PR cleans up several Wave preprocessing problems in NSC and the required Examples reporting path in CI.

  • removes the old blanket pre-preprocess hack that inserted an extra newline after every #pragma
  • keeps the whitespace-aware renderer from fdea055895
  • fixes binary literal handling by enabling Wave support_option_prefer_pp_numbers in waveContext.h instead of manually gluing 0 + b... spellings back together
  • replaces the old generic #pragma workaround with a narrow compatibility normalization for legacy #pragma shader_stage(...) in CHLSLCompiler.cpp
  • fixes #pragma wave dxc_compile_flags(...) parsing so tabs and other whitespace tokens work and the directive is no longer double-counted in waveContext.h
  • fixes EOF handling by normalizing both the root source buffer and loaded include content before they are handed to Wave in CHLSLCompiler.cpp and waveContext.h
  • reports required Examples (...) statuses through a tiny downstream Actions job in build-nabla.yml instead of manual Checks API calls
  • captures the full Examples build and install logs and replays them from that reporter job on failure so the required check is also the readable diagnosis point

Against the current master compare this PR changes build-nabla.yml, CHLSLCompiler.cpp, and waveContext.h. CWaveStringResolver.cpp appears below only as historical context for the bug chronology.

Root cause

1. Binary literals were already wrong before the recent renderer update

  • Builtins such as microfacet_to_light_transform.hlsl use binary literals like 0b01.
  • The original bad reconstruction logic came from c00e5e1b38 by Cyprian Skrzypczak, where Wave output started being rebuilt with raw token spelling dumps in CHLSLCompiler.cpp.
  • 3718dfc596 later moved the same raw token-dump idea into CWaveStringResolver.cpp. That commit relocated already-bad logic. It did not originate the bug.
  • fdea055895 replaced the raw token dump with a whitespace-aware renderer based on Wave's separator detection. That fixed real output issues but also made the older binary-literal problem visible by emitting 0 b01 instead of accidentally gluing it back to 0b01.
  • The right fix is not another manual join rule. The right fix is to let Wave keep 0b01 as a pp-number in the first place via support_option_prefer_pp_numbers.

2. The old #pragma workaround was a broad root-file-only hack

  • Issue #746 was previously papered over in d58cf3a01f by mutating the root source string and inserting an extra newline after every #pragma.
  • That workaround only touched the top-level source buffer. It never applied to included files.
  • The real shader_stage pragma handling already existed in the Wave hook long before that, starting from the original integration by devsh in 0c829039cc3, ff093eee009, and ac21de599dc.
  • What was actually missing was clean compatibility for the legacy Nabla spelling #pragma shader_stage(...) before Wave sees the line.
  • This PR removes the blanket newline hack and replaces it with a narrow normalization step that rewrites only legacy #pragma shader_stage(...) into the already-supported #pragma wave shader_stage(...).

3. dxc_compile_flags had two older parser assumptions

  • The hash_token_occurences based "must be the first directive" bookkeeping came from Cyprian-era Wave hook changes in 7ef1c2fda27, cc8c793b655, and 841ff913d01.
  • The argument parser itself was also assuming a literal " " token separator in changes from 07862397ee5 and ae4386064cf.
  • That was too narrow. Tabs and other whitespace spellings are still whitespace.
  • This PR fixes both sides:
    • directive counting is now done in found_directive only
    • dxc_compile_flags argument splitting now uses Wave whitespace token categories instead of comparing token text to exactly " "

4. EOF handling was already incomplete in the original custom Wave integration

  • The actual origin is 12afd3d42d by Cyprian Skrzypczak. That commit introduced the custom Wave integration directly inside CHLSLCompiler.cpp.
  • In that commit the root source was handed to Wave as wave_context_t context(code.begin(), code.end(), ...) and loaded include content was copied as iter_ctx.instring = res_str;, both without terminal newline normalization.
  • 0c829039cc3, ff9ec3a43d5, and daea09e7cbc by devsh later split and simplified that same model into waveContext.h. Those commits moved the path. They did not create the missing-normalization bug.
  • Wave already runs in support_cpp20, and that already includes support_option_no_newline_at_end_of_file, see language_support.hpp.
  • That option still was not enough for this case because the lexer comment state in cpp.re can trip over EOF before the iterator-level recovery in cpp_iterator.hpp gets a chance to help.
  • On top of that our wrapper treats those exceptions as fatal in CWaveStringResolver.cpp.
  • The correct fix is therefore to normalize the root buffer and loaded include content before lexing starts.

Validation

Local validation on this branch included:

  • cmake --build build/dynamic --target nsc --config Debug -- /m
  • cmake --build build/dynamic --target nsc --config Release -- /m
  • direct Debug and Release nsc compile-to-SPIR-V checks for the EOF include-comment case
  • cmake --build build/dynamic/examples_tests/22_CppCompat --target 22_cppcompat --config Debug -- /m
  • cmake --build build/dynamic/examples_tests/30_ComputeShaderPathTracer --target 30_computeshaderpathtracer --config Debug -- /m
  • cmake --build build/dynamic/examples_tests/31_HLSLPathTracer --target 31_hlslpathtracer --config Debug -- /m
  • cmake --build build/dynamic/examples_tests/66_HLSLBxDFTests --target 66_hlslbxdftests --config Debug -- /m

The local Wave regression matrix now covers:

  • plain EOF include without trailing newline
  • EOF include ending with // comment
  • EOF include ending with /* ... */
  • root source ending with // comment
  • root source ending with a trailing preprocessor directive and // comment
  • legacy #pragma shader_stage(...)
  • binary literals
  • #pragma wave dxc_compile_flags(...) with tab-separated arguments

Thanks

Thanks to @Themperror for surfacing the include, pragma, lexing, and EOF failure cases. Those reports made it much easier to separate the old latent bugs from the recent renderer changes.

@AnastaZIuk AnastaZIuk changed the title Preserve binary literals in Wave output Clean up Wave preprocessor handling Mar 18, 2026
Normalize root and include source buffers before lexing so EOF comments without a trailing newline no longer trip Wave.

Thanks-to: @Themperror for the repros
Fork pull_request runs should stay unprivileged even when Examples statuses are required. The workflow now records the Examples result in the Windows build job and lets a tiny downstream Actions job publish the required check name automatically.

Why: The old checks.create/update path cannot safely publish required Examples statuses for fork pull_request runs.
Why: The reporter job fails closed when build-windows never reaches or never succeeds at the Examples step so required checks do not get stuck as Expected.
The downstream Examples job is now the first place reviewers look when branch protection blocks a PR. It should surface the original build and install output there as well instead of only exposing a pass or fail bit.

Why: The required Examples check should stay self-contained even when it only consumes a status artifact.
Why: Reprinting the captured logs keeps fork pull_request failures readable without any extra GitHub API calls or privileged workflow context.
@AnastaZIuk AnastaZIuk merged commit 3650da9 into Devsh-Graphics-Programming:master Mar 18, 2026
20 of 22 checks 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.

1 participant