Repository navigation
feat: Added Metal binding allocator to supports multiple descriptor sets. - #10
Merged
Merged
Conversation
- Added `metal_binding_allocator.h` and `metal_binding_allocator.cpp` to implement a new allocator that maps Vulkan `(set, binding)` pairs to Metal indices, addressing Metal's fixed resource limits. - Updated `ShaderPipelineBuilder` to utilize the new allocator for program-wide resource management, ensuring consistent Metal index assignments across shader stages. - Enhanced `ShaderArtifactCache` to include a fingerprint for the Metal binding map, improving cache key generation. - Introduced unit tests in `metal_binding_allocator_test.cpp` to validate the allocator's functionality and overflow error reporting for Metal's resource limits. - Refactored related components to support the new allocator, including updates to `spirvcross_cross_compiler` and `spirvcross_reflector` for seamless integration with Metal resource bindings.
- Removed decorative comment lines in CMake files for improved readability and consistency. - Updated comments to be more concise and clear, enhancing the overall clarity of the build configuration. - Adjusted formatting in various CMake files to maintain a uniform style across the project.
…cture - Updated CMake files to include clearer and more structured comments, enhancing readability and maintainability. - Consolidated and organized sections within CMakeLists.txt and related files for better clarity on project structure and dependencies. - Improved formatting consistency across various CMake files, ensuring a uniform style throughout the project. - Added detailed comments for build options, dependencies, and installation rules to facilitate easier understanding for future contributors.
… files - Adjusted formatting in various configuration files, including CMake and GitHub workflows, to ensure consistent use of symbols (e.g., replacing `→` with `->`). - Enhanced comments for better readability and understanding, particularly in CMake files and documentation. - Improved the structure of the `.pre-commit-config.yaml` file for clarity on hook definitions. - Updated README and documentation files to maintain uniformity in style and formatting across the project.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds deterministic program-wide Metal binding allocation, integrates it with shader reflection, MSL cross-compilation, cache fingerprints, and pipeline builds, and adds tests for allocator limits, cache invalidation, cross-stage Metal consistency, and WGSL reflection. It also updates build configuration, release packaging, documentation, and repository metadata. ChangesMetal binding pipeline
Build and repository updates
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
- Replaced `VNE_TEMPLATE_CI` and `VNE_TEMPLATE_LIB_TYPE` with `VNE_SC_CI` in GitHub workflows to align with the new vnesc naming conventions. - Adjusted CMake installation rules to focus on the `vnesc` target, removing unnecessary interface targets and ensuring only relevant binaries are installed. - Enhanced comments in workflow files for better clarity on build configurations and installation processes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/vertexnova/sc/shader_pipeline_builder.cpp (1)
66-165: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftMetal fingerprint requirement forces compile+validate to always run before cache lookup, defeating the point of the artifact cache.
Because
metal_fingerprint(used in the cache key at Line 154) can only be computed once every stage's SPIR-V exists, Phase A now unconditionally callsfront_end_->compile()andvalidator_->validate()for all stages before Phase B'scache->lookup()ever runs — even for a full cache hit. Previously (per the PR's own description) each stage did a cache lookup before compiling.CacheHitSkipsRecompilationstill passes because it only compares resulting SPIR-V, not whether recompilation actually happened, so this regression isn't caught by tests.Consider decoupling the cache key from data that requires compiling first (e.g., a lightweight pre-key from
req.source/macros, with the Metal fingerprint folded in only for a post-hoc integrity check), or accept the trade-off explicitly and document/test it, since as written the cache no longer saves front-end compile/validate cost for MSL builds.Secondary, lower-priority nits in this same block:
StageWork::from_cache(Line 72) is set nowhere and read nowhere — dead field.- The comment at Line 161 ("Keep freshly compiled SPIR-V if cache somehow lacked it...") doesn't match the code below it, which just moves the cached artifact wholesale.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vertexnova/sc/shader_pipeline_builder.cpp` around lines 66 - 165, The cache key currently depends on metal_fingerprint, forcing compile and validation before cache lookup and eliminating cache savings for MSL builds; redesign the lookup flow so cache hits can be identified without compiling every stage first, or explicitly preserve and document this trade-off with regression coverage. While updating this block, remove the unused StageWork::from_cache field and revise the cache-hit comment to accurately describe moving the cached artifact wholesale..github/workflows/codeql.yml (1)
45-79: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winBroken YAML:
steps:list items lost one space of indentation.Every
- name:/- uses:marker in this block sits at 5 spaces while its nested keys (run:,with:,uses:) sit at 8 spaces. That requires the dash at 6 spaces (soname/usesline up with the nested keys' column), not 5. This breaks YAML parsing — actionlint and YAMLlint both report a syntax error at line 46 ("mapping values are not allowed in this context"), and the same misalignment repeats at every step in this job (lines 49, 56, 61, 67, 76), so the entireanalyzejob is unparsable and this workflow will fail to run.🐛 Proposed fix: restore the missing space before each step marker
steps: - - name: Use HTTPS for GitHub (CI runners have no SSH key) + - name: Use HTTPS for GitHub (CI runners have no SSH key) run: git config --global url."https://github.com/".insteadOf "git@github.com:" # vnecmake via submodule; Khronos/gtest via FetchContent at cmake configure. - - uses: actions/checkout@v4 + - uses: actions/checkout@v4 with: submodules: recursive # No build cache: CodeQL must trace a full build. Restoring a cached build # would let cmake --build skip most compilations, so CodeQL would miss # compilation units and analysis would be incomplete. - - name: Install build dependencies + - name: Install build dependencies run: | sudo apt-get update -qq sudo apt-get install -y build-essential cmake - - name: Initialize CodeQL + - name: Initialize CodeQL uses: github/codeql-action/init@v3 with: languages: cpp queries: security-extended - - name: Build (traced by CodeQL) + - name: Build (traced by CodeQL) run: | cmake -B build/static -S . \ ... - - name: Perform CodeQL Analysis + - name: Perform CodeQL Analysis uses: github/codeql-action/analyze@v3 with: category: "/language:cpp"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/codeql.yml around lines 45 - 79, Fix the indentation of every step marker in the CodeQL workflow job by adding one leading space before each “- name:” and “- uses:” entry, including the checkout, dependency installation, initialization, build, and analysis steps. Keep each step’s nested run, with, and uses keys aligned beneath its marker so the steps list parses as valid YAML.Source: Linters/SAST tools
🧹 Nitpick comments (4)
src/vertexnova/sc/crosscompiler/metal_binding_allocator.cpp (1)
122-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate resource-collection logic between the single-compiler constructor and
fromProgram.Lines 126-141 and 160-166 repeat the same six
collect()calls (uniform/storage buffers, sampled/separate/storage images, separate samplers). If a new SPIR-V resource category is added later, it's easy to update one site and forget the other, silently breaking one of the two allocation paths.♻️ Proposed fix: extract a shared collection helper
+void collectAll(const spirv_cross::Compiler& compiler, + std::vector<Key>& buffers, + std::vector<Key>& textures, + std::vector<Key>& combined, + std::vector<Key>& separate) { + const spirv_cross::ShaderResources res = compiler.get_shader_resources(); + collect(compiler, res.uniform_buffers, buffers); + collect(compiler, res.storage_buffers, buffers); + collect(compiler, res.sampled_images, textures); + collect(compiler, res.separate_images, textures); + collect(compiler, res.storage_images, textures); + collect(compiler, res.sampled_images, combined); + collect(compiler, res.separate_samplers, separate); +}Then call
collectAll(compiler, buffers, textures, combined, separate);from both the constructor and thefromProgramloop body.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vertexnova/sc/crosscompiler/metal_binding_allocator.cpp` around lines 122 - 171, Extract the repeated resource collection calls into a shared collectAll helper accepting a compiler and the buffers, textures, combined, and separate vectors. Replace both the constructor’s six collect calls and the equivalent calls inside fromProgram’s loop with collectAll, preserving the existing resource-category mappings.tests/metal_binding_allocator_test.cpp (1)
72-86: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOverflow test only covers pure separate-sampler overflow, not the mixed combined+separate path.
This test only creates
separate_samplers(no combined image-samplers), so it can't catch thehighest(sampler_index_)ordering bug flagged inmetal_binding_allocator.cpp(see that file's review). Consider adding a case that mixes combined image-samplers with separate samplers to exercise that path once the underlying allocator issue is fixed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/metal_binding_allocator_test.cpp` around lines 72 - 86, Extend MetalBindingAllocatorTest.OverflowReportsSamplerLimit to include both combined image-samplers and separate samplers in the same shader, exercising sampler-index ordering through MetalBindingAllocator::fromProgram and overflowError. Preserve the existing assertion that overflowError reports a non-empty message containing “sampler,” while ensuring the mixed bindings exceed the sampler limit.include/vertexnova/sc/sc_types.h (1)
145-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd migration guidance for the new borrowed field.
CrossCompileRequest::metal_program_mapchanges a public aggregate and carries a raw pointer valid only duringShaderPipelineBuilder::build(). Add a migration note explaining the lifetime restriction and that copied requests must not be retained or used asynchronously with this pointer.Based on learnings, public API and core behavior changes should include rationale and migration notes in the PR description.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/vertexnova/sc/sc_types.h` around lines 145 - 161, Add migration documentation for CrossCompileRequest::metal_program_map, stating that it is a non-owning pointer valid only during ShaderPipelineBuilder::build(). Explicitly instruct callers not to retain, copy for later use, or process requests asynchronously while this pointer is set, and include the rationale for the public aggregate/API change in the project’s appropriate migration or PR documentation.Source: Learnings
include/vertexnova/sc/metal_binding_allocator.h (1)
79-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument missing-key behavior for
buffer()/texture()/sampler().Unlike
overflowError()/fingerprint(), these accessors have no doc comment describing what happens when(set, binding)isn't in the map (throw? assert? sentinel value?). Since these are called per-resource by the cross-compiler/reflector, an undocumented contract here risks silent mis-binding if a caller passes an unassigned key.📝 Proposed doc addition
+ /// `@returns` the Metal `[[buffer]]` index for `(set, binding)`. + /// `@pre` `(set, binding)` must be a key produced during construction; behavior for + /// an unknown key is <describe: throws / asserts / returns sentinel>. [[nodiscard]] std::uint32_t buffer(std::uint32_t set, std::uint32_t binding) const; + /// `@returns` the Metal `[[texture]]` index for `(set, binding)`. Same precondition as `@ref` buffer. [[nodiscard]] std::uint32_t texture(std::uint32_t set, std::uint32_t binding) const; + /// `@returns` the Metal `[[sampler]]` index for `(set, binding)`. Same precondition as `@ref` buffer. [[nodiscard]] std::uint32_t sampler(std::uint32_t set, std::uint32_t binding) const;As per coding guidelines, "Use names that reveal intent, keep functions small, and add brief documentation comments to public APIs when behavior is not obvious."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/vertexnova/sc/metal_binding_allocator.h` around lines 79 - 81, Document the missing-key behavior for the public accessors buffer(), texture(), and sampler() in metal_binding_allocator.h, explicitly stating whether each throws, asserts, or returns a sentinel when (set, binding) is absent. Keep the documentation brief and consistent with the existing overflowError() and fingerprint() API comments.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/copilot-instructions.md:
- Around line 9-11: Update the BLOCKER, MAJOR, and MINOR list items in the
surrounding instruction list to use consistent two-space indentation, preserving
their nested-list structure and resolving the markdownlint MD005 violation.
In @.github/workflows/build-reusable.yml:
- Around line 78-79: Correct the indentation of the workflow step sequence in
the build workflow: align the “Use HTTPS for GitHub” step marker and all later
“- name” and “- uses” entries with the surrounding steps: sequence. Preserve
each step’s existing commands and configuration.
In @.github/workflows/ci.yml:
- Around line 55-59: Fix YAML sequence indentation across all affected sites: in
.github/workflows/ci.yml at lines 55-59, 109-122, 134-146, 176-177, 289-299,
309-313, 326-331, and 359-360, align every - name: and - uses: marker with its
nested mapping keys; in .pre-commit-config.yaml at lines 6-13, align - repo:
local, hooks:, and - id: clang-format with their child keys; and in
.github/workflows/release-please.yml at lines 48-58, 95-106, and 108-155, align
matrix entries and publish steps consistently. Preserve each existing key and
value while restoring valid sibling indentation.
In @.github/workflows/delete-merged-branch.yml:
- Around line 32-33: Correct the YAML indentation of the “Delete branch” step so
its “- name” entry aligns with the workflow’s other steps and its nested env
block remains properly scoped. Preserve the existing step configuration and
commands.
In `@cmake/Dependencies.cmake`:
- Around line 37-46: Update _vne_sc_apply_override to clear the corresponding
FETCHCONTENT_SOURCE_DIR_<DEP> cache entry when _cache_var is unset, empty, or
points to a directory without CMakeLists.txt. Preserve the existing forced cache
assignment and status message for valid local overrides, and ensure invalid or
removed paths cannot retain stale FetchContent locations.
In `@src/vertexnova/sc/crosscompiler/metal_binding_allocator.cpp`:
- Around line 61-66: Update lookup() to report a missing Key{set, binding}
instead of returning 0U. Preserve the existing value for found entries, and use
the surrounding allocator’s established assertion or error-reporting mechanism
so mismatched compiler and allocation inputs fail visibly rather than aliasing
index 0.
- Around line 68-70: Update highest() to compute and return the maximum mapped
value by iterating over all entries, rather than returning the value at
m.rbegin(). Preserve the existing 0U result for an empty map, and ensure
overflowError() receives the true highest index for sampler_index_ and other
maps.
In `@src/vertexnova/sc/reflector/spirvcross_reflector.cpp`:
- Around line 110-123: Update the catch-all branch in the reflected resource
mapping around ReflectedResourceType so texture-only resources such as
eSampledImage and eStorageImage assign only metal.texture; remove the
metal.sampler assignment while preserving sampler mapping for eSampler and
eCombinedImageSampler.
In `@src/vertexnova/sc/shader_artifact_cache.cpp`:
- Around line 160-163: The public ShaderArtifactCache::makeKey API removed the
existing three-argument symbol, breaking ABI compatibility for previously built
shared-library clients. Restore a three-argument overload that forwards to the
four-argument makeKey with the appropriate default fingerprint, or explicitly
document the ABI break in the release notes if removal is intentional.
---
Outside diff comments:
In @.github/workflows/codeql.yml:
- Around line 45-79: Fix the indentation of every step marker in the CodeQL
workflow job by adding one leading space before each “- name:” and “- uses:”
entry, including the checkout, dependency installation, initialization, build,
and analysis steps. Keep each step’s nested run, with, and uses keys aligned
beneath its marker so the steps list parses as valid YAML.
In `@src/vertexnova/sc/shader_pipeline_builder.cpp`:
- Around line 66-165: The cache key currently depends on metal_fingerprint,
forcing compile and validation before cache lookup and eliminating cache savings
for MSL builds; redesign the lookup flow so cache hits can be identified without
compiling every stage first, or explicitly preserve and document this trade-off
with regression coverage. While updating this block, remove the unused
StageWork::from_cache field and revise the cache-hit comment to accurately
describe moving the cached artifact wholesale.
---
Nitpick comments:
In `@include/vertexnova/sc/metal_binding_allocator.h`:
- Around line 79-81: Document the missing-key behavior for the public accessors
buffer(), texture(), and sampler() in metal_binding_allocator.h, explicitly
stating whether each throws, asserts, or returns a sentinel when (set, binding)
is absent. Keep the documentation brief and consistent with the existing
overflowError() and fingerprint() API comments.
In `@include/vertexnova/sc/sc_types.h`:
- Around line 145-161: Add migration documentation for
CrossCompileRequest::metal_program_map, stating that it is a non-owning pointer
valid only during ShaderPipelineBuilder::build(). Explicitly instruct callers
not to retain, copy for later use, or process requests asynchronously while this
pointer is set, and include the rationale for the public aggregate/API change in
the project’s appropriate migration or PR documentation.
In `@src/vertexnova/sc/crosscompiler/metal_binding_allocator.cpp`:
- Around line 122-171: Extract the repeated resource collection calls into a
shared collectAll helper accepting a compiler and the buffers, textures,
combined, and separate vectors. Replace both the constructor’s six collect calls
and the equivalent calls inside fromProgram’s loop with collectAll, preserving
the existing resource-category mappings.
In `@tests/metal_binding_allocator_test.cpp`:
- Around line 72-86: Extend
MetalBindingAllocatorTest.OverflowReportsSamplerLimit to include both combined
image-samplers and separate samplers in the same shader, exercising
sampler-index ordering through MetalBindingAllocator::fromProgram and
overflowError. Preserve the existing assertion that overflowError reports a
non-empty message containing “sampler,” while ensuring the mixed bindings exceed
the sampler limit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25c52242-5b97-4687-a30d-2f219f693a3e
📒 Files selected for processing (74)
.github/ISSUE_TEMPLATE/bug_report.md.github/PULL_REQUEST_TEMPLATE.md.github/copilot-instructions.md.github/workflows/build-reusable.yml.github/workflows/ci.yml.github/workflows/codeql.yml.github/workflows/delete-merged-branch.yml.github/workflows/release-please.yml.pre-commit-config.yamlCHANGELOG.mdCMakeLists.txtREADME.mdcmake/CompilerOptions.cmakecmake/Dependencies.cmakecmake/InstallRules.cmakecmake/VnescShaders.cmakedocs/vertexnova/sc/offline_pipeline.mddocs/vertexnova/sc/separate_samplers.mddocs/vertexnova/template/diagrams/README.mdexamples/CMakeLists.txtexamples/README.mdexamples/common/logging_guard.hexamples/glsl/01_hello_compiler/CMakeLists.txtexamples/glsl/02_triangle_bundle/CMakeLists.txtexamples/glsl/03_textured_pipeline_spec/CMakeLists.txtexamples/glsl/CMakeLists.txtinclude/vertexnova/sc/metal_binding_allocator.hinclude/vertexnova/sc/sc_result.hinclude/vertexnova/sc/sc_types.hinclude/vertexnova/sc/shader_artifact_cache.hinclude/vertexnova/sc/shader_bundle.hinclude/vertexnova/sc/shader_compiler_factory.hinclude/vertexnova/sc/shader_cross_compiler.hinclude/vertexnova/sc/shader_pipeline_builder.hinclude/vertexnova/sc/shader_pipeline_spec.hinclude/vertexnova/sc/shader_reflector.hinclude/vertexnova/sc/shader_validator.hinclude/vertexnova/sc/vnesc.hscripts/README.mdscripts/build_linux.shscripts/clang_formatter.pyscripts/generate-docs.shsrc/CMakeLists.txtsrc/vertexnova/sc/crosscompiler/metal_binding_allocator.cppsrc/vertexnova/sc/crosscompiler/spirvcross_cross_compiler.cppsrc/vertexnova/sc/crosscompiler/spirvcross_cross_compiler.hsrc/vertexnova/sc/crosscompiler/tint_cross_compiler.hsrc/vertexnova/sc/frontend/dxc_frontend.hsrc/vertexnova/sc/frontend/glslang_frontend.cppsrc/vertexnova/sc/frontend/glslang_frontend.hsrc/vertexnova/sc/frontend/slang_frontend.hsrc/vertexnova/sc/gpu_layout_tools.cppsrc/vertexnova/sc/reflector/spirvcross_reflector.cppsrc/vertexnova/sc/reflector/spirvcross_reflector.hsrc/vertexnova/sc/shader_artifact_cache.cppsrc/vertexnova/sc/shader_bundle.cppsrc/vertexnova/sc/shader_pipeline_builder.cppsrc/vertexnova/sc/shader_reflection_binary.cppsrc/vertexnova/sc/validator/noop_validator.cppsrc/vertexnova/sc/validator/noop_validator.htests/CMakeLists.txttests/fixtures/glslang_frontend_test_fixture.htests/fixtures/shader_pipeline_builder_test_fixture.htests/fixtures/shader_sources_fixture.htests/metal_binding_allocator_test.cpptests/shader_artifact_cache_test.cpptests/shader_cross_compiler_test.cpptests/shader_pipeline_builder_test.cpptests/shader_pipeline_spec_includes_test.cpptools/README.mdtools/compile_shaders.pytools/download_naga.pytools/shader_compiler_cli/CMakeLists.txttools/shader_compiler_cli/main.cpp
Comment on lines
+160
to
+163
| std::string ShaderArtifactCache::makeKey(const CompileRequest& req, | ||
| const std::vector<CrossTarget>& targets, | ||
| const MetalBindingLayout& metal_layout) { | ||
| const MetalBindingLayout& metal_layout, | ||
| std::uint64_t metal_program_fingerprint) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== candidate files =='
git ls-files | rg '(^|/)shader_artifact_cache\.(h|hpp|cpp|cc|cxx)$|(^|/)ABI|(^|/)version|(^|/)release|(^|/)docs|README'
echo
echo '== locate makeKey =='
rg -n "makeKey\\(" src include . -g '!**/build/**' -g '!**/dist/**'
echo
echo '== inspect shader_artifact_cache files =='
for f in src/vertexnova/sc/shader_artifact_cache.cpp src/vertexnova/sc/shader_artifact_cache.h src/vertexnova/sc/shader_artifact_cache.hpp; do
if [[ -f "$f" ]]; then
echo "--- $f (lines around makeKey) ---"
rg -n -C 4 "makeKey\\(" "$f" || true
fi
done
echo
echo '== search ABI policy mentions =='
rg -n "ABI|binary compat|backward compat|breaking release|mangled|symbol" . -g '!**/build/**' -g '!**/dist/**' -g '!**/.git/**' | head -n 200Repository: vertexnova/vneshaderc
Length of output: 35632
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo 'hello'Repository: vertexnova/vneshaderc
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,180p' include/vertexnova/sc/shader_artifact_cache.h
echo
sed -n '140,220p' src/vertexnova/sc/shader_artifact_cache.cpp
echo
sed -n '1,140p' docs/vertexnova/template/template.mdRepository: vertexnova/vneshaderc
Length of output: 8879
Preserve the 3-arg symbol or call out the ABI break. This public header now exposes only the 4-arg makeKey, so binaries built against the old overload will stop linking in shared-library builds. Add a 3-arg forwarding overload or document the breaking change in the release notes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vertexnova/sc/shader_artifact_cache.cpp` around lines 160 - 163, The
public ShaderArtifactCache::makeKey API removed the existing three-argument
symbol, breaking ABI compatibility for previously built shared-library clients.
Restore a three-argument overload that forwards to the four-argument makeKey
with the appropriate default fingerprint, or explicitly document the ABI break
in the release notes if removal is intentional.
Source: Learnings
…gram fingerprinting - Introduced a new method `makeProgramFingerprint` in `ShaderArtifactCache` to generate a deterministic fingerprint based on compile inputs, improving cache key generation for Metal dense program maps. - Updated `ShaderPipelineBuilder` to utilize the new fingerprinting method, allowing cache lookups to occur before compilation, thus optimizing the shader pipeline process. - Enhanced comments and documentation for clarity on the fingerprinting mechanism and its implications for cache management. - Added unit tests to validate the functionality of the new fingerprinting method and ensure correct behavior across different shader stages.
…and consistency - Adjusted formatting in various GitHub workflow files to ensure consistent indentation and style. - Enhanced comments for better readability and understanding of the CI processes. - Updated the `.pre-commit-config.yaml` file to reflect the new project name and improve hook definitions. - Consolidated and organized steps within workflows to streamline the CI pipeline and enhance maintainability.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/shader_pipeline_builder_test.cpp (1)
225-229: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not skip arbitrary pipeline failures.
At Line 225, a failed result is not evidence that WGSL is unavailable: WGSL cross-compile failures are already non-fatal. This skip can hide front-end, validation, or Metal regressions.
Proposed fix
- // WGSL may be non-fatal missing; if reflection ran, set/binding stay canonical. - if (!result.ok()) { - GTEST_SKIP() << "WGSL path unavailable: " << result.error; - } + ASSERT_TRUE(result.ok()) << result.error;As per coding guidelines, tests must be deterministic and behavior-focused.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shader_pipeline_builder_test.cpp` around lines 225 - 229, Update the test around builder->build(desc) so any failed result is asserted as a test failure rather than skipped. Only accommodate the explicitly non-fatal WGSL-missing behavior already handled by the build pipeline, while preserving the test’s canonical set/binding validation and exposing front-end, validation, and Metal regressions.Source: Coding guidelines
.github/copilot-instructions.md (1)
103-103: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the agent-facing project layout.
Line 103 still directs reviewers to
src/vertexnova/template/andinclude/vertexnova/template/, while this PR uses thevertexnova/scmodule. Rename the section and replace the stale paths so review agents inspect the active vnesc code. Based on supplied PR context, the active public-header path isinclude/vertexnova/sc/.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/copilot-instructions.md at line 103, Update the project-layout guidance in .github/copilot-instructions.md to reference the vertexnova/sc module instead of vertexnova/template: rename the section and replace the source and public-header paths with the active vnesc paths, including include/vertexnova/sc/. Keep the existing tests and examples paths unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-reusable.yml:
- Around line 168-177: The reusable workflow currently accepts lib_type=shared
but the CMake invocation in the build configuration always produces static
archives. Align the interface with the implementation by either removing shared
from the lib_type validation and all callers, or updating the CMake
configuration used by the workflow to enable shared-library builds when lib_type
is shared; ensure cache and build-directory labels cannot claim shared for
static artifacts.
In @.github/workflows/codeql.yml:
- Around line 49-51: Update the actions/checkout@v4 step to set
persist-credentials to false alongside the existing recursive submodules option,
ensuring checkout credentials are removed before repository-controlled CMake
executes.
---
Outside diff comments:
In @.github/copilot-instructions.md:
- Line 103: Update the project-layout guidance in
.github/copilot-instructions.md to reference the vertexnova/sc module instead of
vertexnova/template: rename the section and replace the source and public-header
paths with the active vnesc paths, including include/vertexnova/sc/. Keep the
existing tests and examples paths unchanged.
In `@tests/shader_pipeline_builder_test.cpp`:
- Around line 225-229: Update the test around builder->build(desc) so any failed
result is asserted as a test failure rather than skipped. Only accommodate the
explicitly non-fatal WGSL-missing behavior already handled by the build
pipeline, while preserving the test’s canonical set/binding validation and
exposing front-end, validation, and Metal regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d078d28-e0bb-4a20-be50-79356408965d
📒 Files selected for processing (19)
.github/copilot-instructions.md.github/workflows/build-reusable.yml.github/workflows/ci.yml.github/workflows/codeql.yml.github/workflows/release-please.yml.pre-commit-config.yamlcmake/Dependencies.cmakecmake/InstallRules.cmakeinclude/vertexnova/sc/metal_binding_allocator.hinclude/vertexnova/sc/sc_types.hinclude/vertexnova/sc/shader_artifact_cache.hinclude/vertexnova/sc/shader_pipeline_builder.hsrc/vertexnova/sc/crosscompiler/metal_binding_allocator.cppsrc/vertexnova/sc/reflector/spirvcross_reflector.cppsrc/vertexnova/sc/shader_artifact_cache.cppsrc/vertexnova/sc/shader_pipeline_builder.cpptests/metal_binding_allocator_test.cpptests/shader_artifact_cache_test.cpptests/shader_pipeline_builder_test.cpp
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/metal_binding_allocator_test.cpp
- include/vertexnova/sc/sc_types.h
- include/vertexnova/sc/shader_pipeline_builder.h
- include/vertexnova/sc/metal_binding_allocator.h
- src/vertexnova/sc/shader_artifact_cache.cpp
- cmake/Dependencies.cmake
- src/vertexnova/sc/crosscompiler/metal_binding_allocator.cpp
- src/vertexnova/sc/reflector/spirvcross_reflector.cpp
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/shader_pipeline_builder_test.cpp (1)
225-229: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not skip arbitrary pipeline failures.
At Line 225, a failed result is not evidence that WGSL is unavailable: WGSL cross-compile failures are already non-fatal. This skip can hide front-end, validation, or Metal regressions.
Proposed fix
- // WGSL may be non-fatal missing; if reflection ran, set/binding stay canonical. - if (!result.ok()) { - GTEST_SKIP() << "WGSL path unavailable: " << result.error; - } + ASSERT_TRUE(result.ok()) << result.error;As per coding guidelines, tests must be deterministic and behavior-focused.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shader_pipeline_builder_test.cpp` around lines 225 - 229, Update the test around builder->build(desc) so any failed result is asserted as a test failure rather than skipped. Only accommodate the explicitly non-fatal WGSL-missing behavior already handled by the build pipeline, while preserving the test’s canonical set/binding validation and exposing front-end, validation, and Metal regressions.Source: Coding guidelines
.github/copilot-instructions.md (1)
103-103: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the agent-facing project layout.
Line 103 still directs reviewers to
src/vertexnova/template/andinclude/vertexnova/template/, while this PR uses thevertexnova/scmodule. Rename the section and replace the stale paths so review agents inspect the active vnesc code. Based on supplied PR context, the active public-header path isinclude/vertexnova/sc/.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/copilot-instructions.md at line 103, Update the project-layout guidance in .github/copilot-instructions.md to reference the vertexnova/sc module instead of vertexnova/template: rename the section and replace the source and public-header paths with the active vnesc paths, including include/vertexnova/sc/. Keep the existing tests and examples paths unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-reusable.yml:
- Around line 168-177: The reusable workflow currently accepts lib_type=shared
but the CMake invocation in the build configuration always produces static
archives. Align the interface with the implementation by either removing shared
from the lib_type validation and all callers, or updating the CMake
configuration used by the workflow to enable shared-library builds when lib_type
is shared; ensure cache and build-directory labels cannot claim shared for
static artifacts.
In @.github/workflows/codeql.yml:
- Around line 49-51: Update the actions/checkout@v4 step to set
persist-credentials to false alongside the existing recursive submodules option,
ensuring checkout credentials are removed before repository-controlled CMake
executes.
---
Outside diff comments:
In @.github/copilot-instructions.md:
- Line 103: Update the project-layout guidance in
.github/copilot-instructions.md to reference the vertexnova/sc module instead of
vertexnova/template: rename the section and replace the source and public-header
paths with the active vnesc paths, including include/vertexnova/sc/. Keep the
existing tests and examples paths unchanged.
In `@tests/shader_pipeline_builder_test.cpp`:
- Around line 225-229: Update the test around builder->build(desc) so any failed
result is asserted as a test failure rather than skipped. Only accommodate the
explicitly non-fatal WGSL-missing behavior already handled by the build
pipeline, while preserving the test’s canonical set/binding validation and
exposing front-end, validation, and Metal regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d078d28-e0bb-4a20-be50-79356408965d
📒 Files selected for processing (19)
.github/copilot-instructions.md.github/workflows/build-reusable.yml.github/workflows/ci.yml.github/workflows/codeql.yml.github/workflows/release-please.yml.pre-commit-config.yamlcmake/Dependencies.cmakecmake/InstallRules.cmakeinclude/vertexnova/sc/metal_binding_allocator.hinclude/vertexnova/sc/sc_types.hinclude/vertexnova/sc/shader_artifact_cache.hinclude/vertexnova/sc/shader_pipeline_builder.hsrc/vertexnova/sc/crosscompiler/metal_binding_allocator.cppsrc/vertexnova/sc/reflector/spirvcross_reflector.cppsrc/vertexnova/sc/shader_artifact_cache.cppsrc/vertexnova/sc/shader_pipeline_builder.cpptests/metal_binding_allocator_test.cpptests/shader_artifact_cache_test.cpptests/shader_pipeline_builder_test.cpp
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/metal_binding_allocator_test.cpp
- include/vertexnova/sc/sc_types.h
- include/vertexnova/sc/shader_pipeline_builder.h
- include/vertexnova/sc/metal_binding_allocator.h
- src/vertexnova/sc/shader_artifact_cache.cpp
- cmake/Dependencies.cmake
- src/vertexnova/sc/crosscompiler/metal_binding_allocator.cpp
- src/vertexnova/sc/reflector/spirvcross_reflector.cpp
🛑 Comments failed to post (2)
.github/workflows/build-reusable.yml (1)
168-177: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not accept a
sharedmode that always builds static artifacts.Lines 104-116 accept
lib_type: sharedand label the build/cache directory accordingly, but this configure step no longer passes that choice to CMake and explicitly builds static archives. A caller requestingsharedtherefore gets a static build mislabeled as shared. Either removesharedfrom the reusable-workflow interface and its callers, or restore the vnesc shared-library configuration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-reusable.yml around lines 168 - 177, The reusable workflow currently accepts lib_type=shared but the CMake invocation in the build configuration always produces static archives. Align the interface with the implementation by either removing shared from the lib_type validation and all callers, or updating the CMake configuration used by the workflow to enable shared-library builds when lib_type is shared; ensure cache and build-directory labels cannot claim shared for static artifacts..github/workflows/codeql.yml (1)
49-51: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable persisted checkout credentials before configuring PR code.
The checkout token remains in Git config by default, while Lines 57-73 execute repository-controlled CMake. A malicious CMake change could read and exfiltrate that token. Set
persist-credentials: false.Proposed fix
- uses: actions/checkout@v4 with: submodules: recursive + persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.- uses: actions/checkout@v4 with: submodules: recursive persist-credentials: false🧰 Tools
🪛 zizmor (1.26.1)
[warning] 49-55: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/codeql.yml around lines 49 - 51, Update the actions/checkout@v4 step to set persist-credentials to false alongside the existing recursive submodules option, ensuring checkout credentials are removed before repository-controlled CMake executes.Source: Linters/SAST tools
- Updated the `WgslLeavesVulkanSetBindingIdentity` test to assert successful results from the shader pipeline builder, enhancing test reliability. - Removed the conditional skip for unavailable WGSL paths, ensuring that errors are reported directly in the test output.
…ation - Changed repository name from `VneTemplate` to `vnesc` in the documentation. - Updated project layout section to reflect the new directory structure for the library and public API. - Adjusted CMake subproject commit reference to indicate a dirty state, ensuring accurate version tracking.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Release notes: Use a Conventional Commits–style PR title (e.g.
feat: add X,fix: resolve Y,docs: update Z) so release-please can include this change in the changelog. If you squash-merge, use the PR title as the commit message.Checklist
cmake -B build/staticandcmake --build build/static, or platform script with-l static).ctest --test-dir build/staticor script-a test).clang-formatas configured for this repo); CI clang-format will check.Additional notes
Summary by CodeRabbit
vnescartifact.