Repository navigation
refactor(bazel): patch the kernel defines with a stock Bazel action - #3831
Alexandr-Solovev wants to merge 4 commits into
Conversation
Third step of the Bazel maintainability series. No behavioural change; the Linux build produces the same actions (verified by a full action-cache hit). * `dev/bazel/toolchains/extra_toolchian_lnx.tpl.BUILD` -> `extra_toolchain_lnx`. The typo was load-bearing: the label in `extra_toolchain_lnx.bzl` spelled it the same way, so it worked, but the file did not turn up when grepping for the toolchain by name. * `_copy_dynamic_release_file` in `cc.bzl` had a `cp` branch for non-Windows hosts, and the only two call sites are inside `if is_windows:` and pass `is_windows = is_windows`. Renamed to `_copy_windows_release_file`, dropped the unreachable branch and the flag, and documented why a copy is used instead of a symlink. * `_copy_to_release_impl` in `release.bzl` resolved the `extra` toolchain into a local it never read; the rule does not use the toolchain at all, so the declaration goes too. `daal.bzl` remains its only consumer. * Dropped seven load symbols that are not referenced in the file that loads them. * `.bazelrc` opened with an argument-less `common` line, left behind by the Bazel 8 upgrade (uxlfoundation#3035) when the flag it was meant to carry ended up on the `build` line instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`cc_toolchain_config_lnx.bzl` and `cc_toolchain_config_win.bzl` are two descriptions of two different compiler drivers, so most of their features genuinely differ -- `-I` against `/I`, `-MD -MF` against `/clang:-MD /clang:-MF`. What did not differ was copied: the action groups, ten features whose flags come entirely from rule attributes or Bazel build variables, and the 28 rule attributes both rules declare. Move those to `cc_toolchain_config_common.bzl` and have both files load them. Nothing changes in the generated command lines: building after the change is a full action-cache hit, and every moved block is textually identical to the two originals it replaces, modulo whitespace and the parameter names that stand in for `ctx.attr.*`. `preprocessor_compile_actions` and `codegen_compile_actions` were defined in both files and referenced in neither, so they are dropped instead of moved. Addresses the "Toolchain code unification" item in dev/bazel/TODO.md. The per-compiler features stay where they are; a shared factory with a per-platform branch inside would be harder to follow than the duplicate it replaces. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removing one `#define` per disabled ISA from `services/internal/daal_kernel_defines.h` was implemented as a custom toolchain: a toolchain type, a provider, a repository rule, a module extension, two generated `BUILD` templates and three copies of the patcher itself (bash, batch, PowerShell). Ten files, so that an action could run one regex substitution. `ctx.actions.expand_template` performs the substitution at execution time, so nothing needs to read the header during analysis and no helper tool is needed on either platform. The Makefile's Windows line ending -- a disabled define becomes a lone CR, leaving `\r\n` -- is expressed as the replacement string. Also removes a real hermeticity bug: the patcher was handed to `ctx.actions.run` as an `attr.string` absolute path, not as a file, so it was not an input of the action and editing it did not invalidate the cached output. Verified byte for byte against the previous implementation's output for `--cpu=all`, `avx512`, `avx2` and `sse2`, and the following build is a full action-cache hit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The stacked cross-platform toolchain refactor includes Windows-specific behavior without CI coverage and warrants final human validation.
Review effort: Balanced
Findings: None
What changed in this PR
Replaces the custom kernel-definition patching toolchain with Bazel’s hermetic expand_template action, while incorporating the stacked shared toolchain configuration refactor.
Changes:
- Patches disabled ISA defines directly with a tracked Bazel action.
- Removes the extra toolchain, platform wrappers, templates, and scripts.
- Consolidates shared Linux/Windows C++ toolchain configuration.
| File | Description |
|---|---|
.bazelrc |
Removes empty configuration and trailing whitespace. |
MODULE.bazel |
Removes extra toolchain registration. |
dev/bazel/cc.bzl |
Restricts release-copy helper to Windows. |
dev/bazel/cc/common.bzl |
Removes an unused utility import. |
dev/bazel/cc/link.bzl |
Removes an unused utility import. |
dev/bazel/daal.bzl |
Uses expand_template to patch ISA defines. |
dev/bazel/release.bzl |
Removes the unused extra toolchain dependency. |
dev/bazel/toolchains/BUILD |
Removes the obsolete toolchain type. |
dev/bazel/toolchains/cc_toolchain_config_common.bzl |
Adds shared toolchain features and attributes. |
dev/bazel/toolchains/cc_toolchain_config_lnx.bzl |
Uses the shared Linux/Windows configuration. |
dev/bazel/toolchains/cc_toolchain_config_win.bzl |
Uses the shared Linux/Windows configuration. |
dev/bazel/toolchains/extra_toolchian_lnx.tpl.BUILD |
Deletes the Linux extra-toolchain template. |
dev/bazel/toolchains/extra_toolchain.bzl |
Deletes the custom toolchain implementation. |
dev/bazel/toolchains/extra_toolchain_extension.bzl |
Deletes the module extension. |
dev/bazel/toolchains/extra_toolchain_lnx.bzl |
Deletes Linux repository setup. |
dev/bazel/toolchains/extra_toolchain_win.bzl |
Deletes Windows repository setup. |
dev/bazel/toolchains/extra_toolchain_win.tpl.BUILD |
Deletes the Windows extra-toolchain template. |
dev/bazel/toolchains/tools/patch_daal_kernel_defines.cmd |
Deletes the Windows launcher. |
dev/bazel/toolchains/tools/patch_daal_kernel_defines.ps1 |
Deletes the Windows patcher. |
dev/bazel/toolchains/tools/patch_daal_kernel_defines.sh |
Deletes the Linux patcher. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
napetrov
left a comment
There was a problem hiding this comment.
Measured locally: Linux x86, Bazel 9.2.0, gcc 13.3, icpx 2025.2; clean env (env -i, no MKLROOT/TBBROOT); one worktree + output base per head, base = merge-base 3120d0ab.
- Patched
daal_kernel_defines.his byte-identical to base for--cpu=all,avx512,avx2,sse2,"avx2 avx512",auto. Walking #3830 -> this head in one output base re-runs only 2 internal actions; the other 3225 are cache hits. - The
LinuxBazelGNU_OpenBLAS_rv64failure (Azure build 63735, install bazel step) is:error: Failed to fetch Bazelisk release information from GitHub API.- a rate limit, unrelated to this diff. #3835 hit the same thing. - Nit: the description says Windows has no CI coverage, but
WindowsBazelran on this head and was green, so the\rpath is exercised.
| template = ctx.file.src, | ||
| output = kernel_defines, | ||
| substitutions = { | ||
| "#define DAAL_KERNEL_{}".format(cpu.upper()): "\r" if is_windows else "" |
There was a problem hiding this comment.
This is a literal, unanchored replace, so it also hits longer macro names and any other mention of the macro. Demonstrated by adding two lines to the header and building with --cpu=sse2:
#define DAAL_KERNEL_AVX2_VNNI -> _VNNI
/* mirrors #define DAAL_KERNEL_AVX512 */ -> /* mirrors */
Make's sed 's/^#define DAAL_KERNEL_X\b//' leaves both lines alone. The docstring's "mentions those macros nowhere else" holds today but nothing enforces it; it breaks silently the day someone adds an AVX2_VNNI / AVX512_* style variant.
Anchoring on the whole line keeps it equivalent, e.g. key "\n#define DAAL_KERNEL_{}\n".format(cpu.upper()) -> "\n\r\n" if is_windows else "\n\n" (the header is eol=lf per .gitattributes, and no define sits on line 1).
There was a problem hiding this comment.
Agreed, fixed in ab76a83: the key is now "\n#define DAAL_KERNEL_<ISA>\n" -> "\n\n" ("\n\r\n" on Windows), and the docstring is updated. I checked it with your two lines added and --cpu=sse2: AVX2 and AVX512 are blanked (adjacent defines still match one after another, since each replacement keeps its trailing newline), and #define DAAL_KERNEL_AVX2_VNNI and the /* mirrors ... */ comment pass through unchanged.
A literal key also matched longer macro names such as DAAL_KERNEL_AVX2_VNNI and mentions inside comments; sed's ^...\b never did. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Superseded by #3833, which now carries this PR's commits unchanged (plus the review fixes made here), rebased on current |
Fourth PR of the
dev/bazelsimplification series. Stacked on #3830 — review that one first; the diff below is the last commit only.What this removes
cpp/daal/include/services/internal/daal_kernel_defines.henables every CPU dispatch variant oneDAL can build, and the ISAs that--cpuleaves out have to be stripped from it before it is compiled or released. That one regex substitution was implemented as a custom Bazel toolchain:toolchains/extra_toolchain.bzltoolchainruletoolchains/extra_toolchain_lnx.bzl,_win.bzltoolchains/extra_toolchain_extension.bzltoolchains/extra_toolchain_{lnx,win}.tpl.BUILDBUILDtemplatestoolchains/tools/patch_daal_kernel_defines.{sh,cmd,ps1}toolchain_type(name = "extra"), 3 lines inMODULE.bazelTen files, a custom toolchain type and a module extension, so that one action could run one
sed.ctx.actions.expand_templateis the stock Bazel action for exactly this: it performs literal substitutions on a file at execution time, so nothing has to read the header during analysis and no helper tool is needed on either platform. The whole mechanism collapses to:The Makefile's Windows behaviour —
sed.eolis empty on Linux and a lone CR on Windows, so a disabled define leaves an empty line with the platform's own line ending — is expressed as the replacement string rather than as a separate script.net -304 / +33lines, and thedev/bazel/toolchainsdirectory loses its second toolchain type.Hermeticity fix
The old rule received the patcher through
ExtraToolchainInfoas anattr.stringabsolute path and passed it toctx.actions.run(executable = ...)without ever declaring it as an input. Editingpatch_daal_kernel_defines.shtherefore did not invalidate the cached output. The new implementation has no tool at all, so the only inputs are the header and the--cpusetting, both of which Bazel already tracks.Validation
--cpu=all,--cpu=avx512,--cpu=avx2and--cpu=sse2(captured before the change, compared after).bazel build //cpp/daal:core_static //cpp/oneapi/dal/algo:kmeans_dpcis a full action-cache hit against the pre-change cache — no command line changed.bazel build --nobuild //:release //:release_all //examples/oneapi/cpp:allanalyses clean.extra_toolchain,ExtraToolchainInfo,patch_daal_kernel_definesor//dev/bazel/toolchains:extra.The Windows path has no CI coverage here; it is unchanged in intent and the replacement string reproduces what
patch_daal_kernel_defines.ps1wrote.