forked from KhronosGroup/SPIRV-Tools
-
Notifications
You must be signed in to change notification settings - Fork 0
roll deps #1
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
Open
github-actions
wants to merge
1,814
commits into
main
Choose a base branch
from
roll_deps
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
roll deps #1
Conversation
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
github-actions
bot
commented
Jan 28, 2026
- Roll external/re2/ e7aec5985..972a15ced (2 commits)
- Roll external/googletest/ 85087857a..56efe3983 (2 commits)
- Roll external/abseil_cpp/ 889ddc99e..0437a6d16 (3 commits)
When building via CMake (SPIRV_TOOLS_FFI_SKIP_CPP_LINK=1), don't define SPIRV_RUST_TARGET_ENV in the Rust FFI build. This allows context_bridge.cc to include its own implementations of dispatch_context_message and assemble_text_with_context, avoiding a circular dependency between SPIRV-Tools and spirv-tools-ffi at link time. Previously, SPIRV_RUST_TARGET_ENV was always defined, which caused the stub implementations in context_bridge.cc to be skipped, expecting text.cpp to provide them. But since spirv-tools-ffi is linked into SPIRV-Tools, and text.cpp is part of SPIRV-Tools, this created a circular dependency that the linker couldn't resolve. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The previous fix used SPIRV_TOOLS_FFI_SKIP_CPP_LINK to determine whether to define SPIRV_RUST_TARGET_ENV, but both Bazel and CMake set this variable. This caused the Bazel build to fail because it tried to include generated SPIRV-Tools headers that don't exist in the sandbox. This fix adds a new environment variable SPIRV_RUST_TARGET_ENV_DEFINE that Bazel sets to explicitly request Rust-only implementations. The logic is now: - Standalone cargo build: Define SPIRV_RUST_TARGET_ENV, use Rust stubs - Bazel build: Define SPIRV_RUST_TARGET_ENV, use Rust stubs - CMake build: Don't define, use C++ implementations with C++ libraries 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
When compiling context_bridge.cc via cargo (build.rs), we must always define SPIRV_RUST_TARGET_ENV. This ensures: 1. We skip C++ includes that need generated headers (reducer.h, libspirv.hpp) 2. We delegate to Rust implementations (validate_binary_rust, etc.) 3. We don't provide duplicate implementations of dispatch_context_message and assemble_text_with_context (text.cpp provides these in CMake builds when SPIRV_RUST_TARGET_ENV is defined there too) The previous conditional logic caused multiple definition errors because in CMake builds, both context_bridge.cc and text.cpp were providing implementations of the same functions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Changes: 1. build.rs: Only define SPIRV_RUST_TARGET_ENV when SPIRV_RUST_TARGET_ENV_DEFINE=1 (Bazel). In CMake builds, context_bridge.cc provides implementations directly. 2. context_bridge.cc: Include source/table.h and properly implement dispatch_context_message using context->consumer. This allows the Rust FFI library to provide message dispatch without depending on linker order. 3. text.cpp: Remove duplicate FFI implementations. context_bridge.cc now provides these in all build scenarios. 4. build_rust_ffi.py: Handle Windows library extension (.lib vs .a). This fixes: - Undefined reference errors in CMake builds (linker ordering) - Multiple definition errors when both files provided implementations - Bazel Windows builds failing to find the output library 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
When building the Rust FFI library via CMake, we need to set SPIRV_RUST_TARGET_ENV_DEFINE=1 so that context_bridge.cc compiles with stub implementations that don't depend on SPIRV-Tools-reduce. Without this, context_bridge.cc includes full C++ implementations that reference the Reducer class, but the shared library doesn't link against SPIRV-Tools-reduce, causing undefined symbol errors. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
These FFI functions only depend on the public C API and table.h struct definitions, not on the Reducer library. Move table.h and libspirv.hpp includes outside the SPIRV_RUST_TARGET_ENV guard so these functions are always compiled. Only the Reducer-specific headers (reducer.h, spirv_reducer_options.h) remain guarded since they're only needed by reduce_with_cpp. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
When SPIRV_RUST_TARGET_ENV is defined (CMake builds), provide a stub implementation of assemble_text_with_context that returns failure. This avoids calling spvTextToBinaryWithOptions from within libspirv_tools_ffi, which would create a circular dependency since SPIRV-Tools links against spirv-tools-ffi. In CMake builds, the Rust side should use its native assembler instead of calling back into C++. The stub signals this by returning failure. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
When building with SPIRV_ENABLE_RUST_TARGET_ENV, the shared library includes Rust FFI symbols from the cxx crate. Update the symbol export check to allow: - cxxbridge1$ : CXX bridge runtime symbols - spvtools$ffi$ : SPIRV-Tools Rust FFI symbols - _R : Rust v0 mangled symbols 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add rust_ prefix to the allowed symbol patterns to allow rust_eh_personality and other Rust runtime symbols. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The Rust standard library depends on ws2_32 (Windows Sockets), ntdll (NT API), userenv, and bcrypt system libraries. Add these as INTERFACE_LINK_LIBRARIES on Windows so consumers of the spirv-tools-ffi imported target get them linked automatically. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The Bazel sandbox blocks cargo from accessing ~/.cargo/git/ to fetch git dependencies like rspirv, causing the build to fail with "Operation not permitted". Setting local = True disables sandboxing for this genrule, allowing cargo to fetch dependencies normally. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
When SPIRV_RUST_TARGET_ENV is defined, avoid including source/table.h and other internal headers that depend on generated files like core_tables_header.inc. Instead, provide stub implementations for all FFI functions including dispatch_context_message. This fixes the Bazel build which doesn't have the generated headers available when building the Rust FFI library. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
When spirv-tools-core is used as a git dependency, the SPIRV-Headers submodule isn't available. This adds support for the SPIRV_HEADERS_PATH environment variable to specify an alternative location for the headers. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
When spirv-tools-core is used as a cargo git dependency, submodules are not cloned, so the grammar file at external/spirv-headers/ is unavailable. Vendor a copy under grammar/ and update build.rs to fall back to it when the submodule path does not exist. Resolution order: SPIRV_HEADERS_PATH env var > submodule > vendored.
Add dedicated mem2reg.egg and licm.egg rule files extracted from other rule files. Add disabled rule files (merge_return, loop_fusion, legalization, instrumentation) with explanatory comments. Expand datatypes.egg with legalization types. Add 15 unit tests for mem2reg and LICM passes.
Rewrite porting-plan.md with accurate validator module listing (49 rule modules), optimizer file inventory (32 files, 28 enabled), correct test file names, and removal of non-existent scripts.
7a63940 to
dfbaab5
Compare
Match C++ spirv-val behavior: capabilities enabled by declared extensions should bypass SPIR-V version requirements. Previously, VulkanMemoryModel had a special-case early return that rejected it before checking extensions. Changes: - Remove VulkanMemoryModel early return in capability validation - Add extension check to operand version validation (matching C++ OperandVersionExtensionCheck) so that declared extensions like SPV_KHR_vulkan_memory_model can relax SPIR-V version requirements for operands like VulkanKHR memory model - Add test for Vulkan 1.1 + VulkanMemoryModel with extension - Update test expectations to reflect more precise error types
The assembler now checks the result type when encoding OpConstant operands. Integer text like "42" for a float32 type is interpreted as the float value 42.0 and stored as its IEEE 754 bit pattern, matching the C++ spirv-as behavior for LiteralContextDependentNumber. Float text like "42.5" is also accepted for float types by falling back to OperandValue::Word in the parser when integer parsing fails.
The assembler used Builder::function_parameter() which auto-generates IDs from Builder's internal next_id counter. This counter is separate from the module_builder's next_numeric_id counter used by all other instructions, leading to ID collisions (e.g., OpFunction and its OpFunctionParameter both getting the same result ID). Fix by pre-allocating the ID through module_builder.bind_typed_result() and constructing the OpFunctionParameter instruction manually, matching the pattern used by all other translate methods.
google/re2@e7aec59...972a15c Created with: roll-dep external/re2
google/googletest@8508785...56efe39 Created with: roll-dep external/googletest
abseil/abseil-cpp@889ddc9...49cd3c7 Created with: roll-dep external/abseil_cpp
dfbaab5 to
87eccd1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.