Avoid duplicate core module builds#11721
Conversation
📝 WalkthroughWalkthroughAdds ChangesAvoid redundant core module builds
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 87c6bffe-f69c-47e9-8f05-d07cbd6f73d5
📒 Files selected for processing (6)
cmake/RunIfOutdated.cmakeexamples/cpu-shader-llvm/CMakeLists.txtsource/slang-core-module/CMakeLists.txtsource/slang-glsl-module/CMakeLists.txtsource/slang/slang-options.cppsource/standard-modules/neural/CMakeLists.txt
| foreach(input IN LISTS SLANG_RUN_IF_OUTDATED_INPUTS) | ||
| foreach(output IN LISTS SLANG_RUN_IF_OUTDATED_OUTPUTS) | ||
| if("${input}" IS_NEWER_THAN "${output}") | ||
| set(SLANG_RUN_IF_OUTDATED_NEEDED TRUE) | ||
| endif() | ||
| endforeach() | ||
| endforeach() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail fast when a declared staleness input is missing.
On Line 16, inputs are compared with IS_NEWER_THAN, but there is no existence check for each input path first. If an input path is wrong/missing, this can silently evaluate as up-to-date and skip regeneration incorrectly.
🔧 Proposed fix
if(NOT SLANG_RUN_IF_OUTDATED_NEEDED)
foreach(input IN LISTS SLANG_RUN_IF_OUTDATED_INPUTS)
+ if(NOT EXISTS "${input}")
+ message(FATAL_ERROR "SLANG_RUN_IF_OUTDATED_INPUTS entry does not exist: ${input}")
+ endif()
foreach(output IN LISTS SLANG_RUN_IF_OUTDATED_OUTPUTS)
if("${input}" IS_NEWER_THAN "${output}")
set(SLANG_RUN_IF_OUTDATED_NEEDED TRUE)
endif()
endforeach()
endforeach()
endif()As per path instructions, "cmake/**: Build system changes. Verify cross-platform compatibility (Windows/Linux/macOS). Check for proper target dependency declarations and generator expressions."
Source: Path instructions
33693ba to
adc99f3
Compare
Fixes #11717
Motivation
When
SLANG_EMBED_CORE_MODULE=OFF, a debug build was compiling the core module multiple times. The dedicated core module generation step produced embedded headers, but downstream module and example build rules still invoked compiler paths that had no prebuilt core archive, soslang-bootstraporslangccompiled core again while generating GLSL, neural, andexamples/cpu-shader-llvm/shader.oartifacts.Proposed solution
Make the generated core module archive a first-class build output. Downstream module compilation now consumes it through
-load-core-module, and saved built-in module outputs are deferred so a command such asslang-bootstrap -load-core-module <archive> -save-glsl-module-bin-source ...compiles only the requested non-core built-in module. This keeps the dependency at CMake rule boundaries instead of relying on each downstream compiler invocation to rebuild core independently.Change summary
source/slang-core-module/CMakeLists.txtnow writesslang-core-module.binand the core header from one rule, exposes them through onegenerate_core_moduletarget, and generates the GLSL header from a second rule that loads that archive.cmake/RunIfOutdated.cmakewraps build-time generation commands so repeated Visual Studio target traversal skips commands whose declared outputs are newer than their inputs.source/slang/slang-options.cppdefers save requests until after-compile-core-module, and compiles a requested built-in module on demand when a save request targets a module that has not been loaded yet.source/standard-modules/neural/CMakeLists.txtusesslang-bootstrapand loads the generated core archive, avoiding a build-time dependency onslangc.examples/cpu-shader-llvm/CMakeLists.txtpasses the generated core archive to itsslangcshader-object command, avoiding a second core compile during full builds.source/slang-glsl-module/CMakeLists.txtdepends on the GLSL generation target instead of the aggregate core-module header target.Concepts and vocabulary
slang-core-module.binartifact loaded by-load-core-modulewhen embedded core is off.slang-bootstrap, the standalone generator tool used during the build before the fullslangcexecutable is required.Process report
The root cause was not that CMake had too few dependencies; it was that some consumers only depended on generated headers while invoking compiler commands without an explicit core archive. The first fix in
source/slang-core-module/CMakeLists.txtseparates core archive generation from GLSL header generation: the core rule runsslang-bootstrap -compile-core-module -save-core-module ..., and the GLSL rule runsslang-bootstrap -load-core-module ... -save-glsl-module-bin-source .... This makes the core archive the single source of truth for downstream generator commands.The compiler option handling previously saved built-in module outputs as each option was parsed. That ordering forced combined command lines such as
-compile-core-module -save-core-module-bin-sourceto depend on parse order and made it hard to save a non-core module after loading an existing core archive.OptionsParser::addPendingBuiltinModuleSaveandOptionsParser::writePendingBuiltinModuleSavesrecord save requests, run them after the optional core compile, and callcompileBuiltinModuleonly for the named module whensaveBuiltinModulereports that it is missing. A request to save GLSL after-load-core-moduleis therefore a valid input shape: the core module is already loaded, and only the requested dependent module needs to be built.Neural was using the full compiler for build-time module generation. That was the wrong layer for this build rule because neural only needs a standalone generator plus the prebuilt core archive.
source/standard-modules/neural/CMakeLists.txtnow selectsslang-bootstrapand depends ongenerate_core_module, so building neural does not requireslangc.exeand does not trigger core compilation.The remaining full-build duplication came from
examples/cpu-shader-llvm/CMakeLists.txt. Itsslangcinvocation forshader.ohad no-load-core-module, soslangchad to compile core internally even after the dedicated archive rule had completed. Adding-load-core-module ${core_module_archive}and the corresponding dependency makes that example consume the same build artifact as the module-generation rules.cmake/RunIfOutdated.cmakeaddresses repeated generator-rule traversal from Visual Studio builds. The CMake targets may still be visited more than once through different dependencies, but the wrapper checks declared outputs against explicit inputs and skips the command when the generated artifacts are up to date. This keeps the build graph behavior deterministic without relying on undocumented command-side side effects.