Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions .bazelrc
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
# Global options

common

build -c opt \
--incompatible_enable_cc_toolchain_resolution \
--incompatible_require_linker_input_cc_api
--incompatible_require_linker_input_cc_api

# Aliases for user-defined flags
build --flag_alias=backend_config=@config//:backend_config
Expand Down
6 changes: 0 additions & 6 deletions MODULE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,6 @@ sh_config_ext = use_extension("@onedal//dev/bazel/toolchains:cc_toolchain_extens
use_repo(sh_config_ext, "onedal_cc_toolchain")
register_toolchains("@{}//:all".format("onedal_cc_toolchain"))


extra_toolchain_ext = use_extension("@onedal//dev/bazel/toolchains:extra_toolchain_extension.bzl", "onedal_extra_toolchain_extension")
use_repo(extra_toolchain_ext, "onedal_extra_toolchain")
register_toolchains("@{}//:all".format("onedal_extra_toolchain"))


http_archive = use_repo_rule("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
http_archive(
name = "catch2",
Expand Down
63 changes: 34 additions & 29 deletions dev/bazel/cc.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
#===============================================================================

load("@onedal//dev/bazel:utils.bzl",
"utils",
"paths",
"sets",
)
Expand Down Expand Up @@ -317,31 +316,39 @@ cc_static_lib = rule(
)


def _copy_dynamic_release_file(ctx, src, out_name, is_windows = False, extra_inputs = []):
def _copy_windows_release_file(ctx, src, out_name, extra_inputs = []):
"""Copy a linker output under the file name the release layout expects.

Windows only: the Linux release names come straight out of the link action,
while a DLL and its import library have to be renamed afterwards (see
`_cc_dynamic_lib_impl`). A symlink is not enough, because creating one
requires developer mode on Windows.

Args:
ctx: rule context.
src: the file produced by the link action.
out_name: base name of the copy, declared in the current package.
extra_inputs: further link outputs to declare as inputs, so that the
copy cannot run before the whole link action completed.

Returns:
The declared copy.
"""
out = ctx.actions.declare_file(out_name)
if is_windows:
ctx.actions.run(
executable = "cmd.exe",
inputs = [src] + extra_inputs,
outputs = [out],
arguments = [
"/d",
"/c",
'copy /Y "{}" "{}"'.format(
src.path.replace("/", "\\"),
out.path.replace("/", "\\"),
),
],
use_default_shell_env = True,
)
else:
ctx.actions.run(
executable = "cp",
inputs = [src] + extra_inputs,
outputs = [out],
arguments = [src.path, out.path],
use_default_shell_env = True,
)
ctx.actions.run(
executable = "cmd.exe",
inputs = [src] + extra_inputs,
outputs = [out],
arguments = [
"/d",
"/c",
'copy /Y "{}" "{}"'.format(
src.path.replace("/", "\\"),
out.path.replace("/", "\\"),
),
],
use_default_shell_env = True,
)
return out


Expand Down Expand Up @@ -407,19 +414,17 @@ def _cc_dynamic_lib_impl(ctx):
if dynamic_outputs.dynamic_library.basename == dynamic_release_name:
default_files.append(dynamic_outputs.dynamic_library)
else:
default_files.append(_copy_dynamic_release_file(
default_files.append(_copy_windows_release_file(
ctx,
dynamic_outputs.dynamic_library,
dynamic_release_name,
is_windows = is_windows,
extra_inputs = [dynamic_outputs.interface_library] if dynamic_outputs.interface_library else [],
))
if dynamic_outputs.interface_library:
default_files.append(_copy_dynamic_release_file(
default_files.append(_copy_windows_release_file(
ctx,
dynamic_outputs.interface_library,
"{}_dll.lib".format(rt_name),
is_windows = is_windows,
))
default_info = DefaultInfo(
files = depset(default_files),
Expand Down
1 change: 0 additions & 1 deletion dev/bazel/cc/common.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ load("@rules_cc//cc/common:cc_info.bzl", "CcInfo")

load("@onedal//dev/bazel:utils.bzl",
"utils",
"paths",
"sets",
)

Expand Down
87 changes: 49 additions & 38 deletions dev/bazel/cc/link.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,8 @@ load("@rules_cc//cc:action_names.bzl", "ACTION_NAMES")
load("@onedal//dev/bazel:utils.bzl",
"utils",
"paths",
"sets",
)

load("@onedal//dev/bazel/toolchains:action_names.bzl",
"CPP_MERGE_STATIC_LIBRARIES"
)
load("@onedal//dev/bazel/cc:common.bzl",
onedal_cc_common = "common",
)
Expand All @@ -45,61 +41,71 @@ def _filter_user_link_flags(feature_configuration, user_link_flags):

def _merge_static_libs(filename, actions, cc_toolchain,
feature_configuration, static_libs, is_windows = False):
"""Archive object files and whole static libraries into a single library.

`cc_common` has no API for this: its archiving action adds an input
archive as a member instead of copying the members out of it, so the
archiver has to be driven directly.

On Windows `lib` takes the inputs on its command line. GNU `ar` instead
needs an MRI script on stdin, which `actions.write` produces and the
shell redirects; that keeps the archiver the toolchain's own, tracked
`cpp_link_static_library` tool rather than a wrapper script generated by
the repository rule.

Args:
filename: basename of the library to produce.
actions: the rule context's `actions`.
cc_toolchain: the resolved `CcToolchainInfo`.
feature_configuration: the configured features, used to look up the
archiver.
static_libs: object files and static libraries to merge.
is_windows: whether the target platform is Windows.

Returns:
The merged library `File`.
"""
output_file = actions.declare_file(filename)
archiver_path = cc_common.get_tool_for_action(
feature_configuration = feature_configuration,
action_name = ACTION_NAMES.cpp_link_static_library,
)
if is_windows:
merger_path = cc_common.get_tool_for_action(
feature_configuration = feature_configuration,
action_name = ACTION_NAMES.cpp_link_static_library,
)
args = actions.args()
args.use_param_file("@%s", use_always = True)
args.set_param_file_format("multiline")
args.add("/NOLOGO")
args.add("/OUT:" + output_file.path)
args.add_all(static_libs)
actions.run(
executable = merger_path,
executable = archiver_path,
arguments = [args],
inputs = static_libs,
outputs = [output_file],
mnemonic = "MergeStaticLibraries",
use_default_shell_env = True,
)
return output_file
merger_path = cc_common.get_tool_for_action(
feature_configuration = feature_configuration,
action_name = CPP_MERGE_STATIC_LIBRARIES,
)
merger_variables = cc_common.create_link_variables(
feature_configuration = feature_configuration,
cc_toolchain = cc_toolchain,
is_using_linker = False,
)
command_line = cc_common.get_memory_inefficient_command_line(
feature_configuration = feature_configuration,
action_name = CPP_MERGE_STATIC_LIBRARIES,
variables = merger_variables,
)
args = actions.args()
args.add_all(command_line)
args.add(output_file)
args.add_all(static_libs)
env = cc_common.get_environment_variables(
feature_configuration = feature_configuration,
action_name = CPP_MERGE_STATIC_LIBRARIES,
variables = merger_variables,
mri_script = actions.declare_file(filename + ".mri")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coverage note: on x86 with MKL this branch never runs. aquery 'mnemonic("MergeStaticLibraries", ...)' finds 0 actions for host //:release, DPC //:release, and //cpp/... + //examples/.... It only fires when a prebuilt .a is in the linking context, i.e. --backend_config=ref (libonedal_thread.a <- libonedal_thread_no_deps.a + libopenblas.a).

I built static OpenBLAS 0.3.34 with .ci/env/openblas.sh and ran that config: the MRI script works with the bzlmod + paths and the release tree is byte-identical to base. So the aarch64/rv64 OpenBLAS jobs are the only CI coverage of this code - worth stating in the description.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed independently, and agreed it belongs in the description.

Reproduced the 0-action result here on x86 with MKL:

aquery 'mnemonic("MergeStaticLibraries", //:release)'                        -> 0 actions
aquery 'mnemonic("MergeStaticLibraries", //cpp/... union //examples/...)'    -> 0 actions

And pinned down exactly where the flag that does reach it lives — --backend_config=ref appears in two places in CI and nowhere else:

  • .github/workflows/ci-aarch64.yml lines 253, 305, 330 — the LinuxBazel_OpenBLAS(SVE) jobs
  • .ci/pipeline/ci.yml line 717 — CI (LinuxBazelGNU_OpenBLAS_rv64)

So your conclusion holds: the aarch64 and rv64 OpenBLAS jobs are the only CI coverage of the MRI branch. The x86 jobs exercise the Windows lib branch and the unchanged link paths instead. I could not re-run the ref config locally — this worktree has no static OpenBLAS, and aquery --backend_config=ref stops at Cannot locate +openblas_repo+openblas dependency — so I am taking your byte-identical ref release tree on report rather than claiming it myself.

Your docstring point is also right, and it was wrong in both the summary line and the Args: entry. Only lines 121 and 252 pass all_object_list, and both sit inside if is_windows; the Linux-reachable call sites at 150, 198 and 207 pass unpacked_linking_context.static_libraries alone. Fixed in 3497aba:

    """Archive whole static libraries into a single library.
    ...
    On Linux `static_libs` holds archives only, since object files go
    through `cc_common` first.

    Args:
        ...
        static_libs: static libraries to merge, plus loose object files on
                     Windows.

//:release still analyses clean after it. Docstring-only, so no action re-execution.

The coverage paragraph above is not in the description yet — I will get it in there.

actions.write(
output = mri_script,
content = "\n".join(
["CREATE " + output_file.path] +
["ADDLIB " + lib.path for lib in static_libs] +
["SAVE", ""]
),
)
actions.run(
executable = merger_path,
arguments = [args],
env = env,
actions.run_shell(
command = '"{}" -M < "{}"'.format(archiver_path, mri_script.path),
inputs = depset(
direct = static_libs,
direct = static_libs + [mri_script],
transitive = [
cc_toolchain.all_files,
],
),
outputs = [output_file],
mnemonic = "MergeStaticLibraries",
progress_message = "Merging static libraries into %{output}",
)
return output_file

Expand Down Expand Up @@ -298,8 +304,13 @@ def _link(owner, name, actions, cc_toolchain,
# @file spelling is a response file, not a DEF file, and causes the
# Windows linker to export symbols discovered from whole archives
# instead of the explicit Make-compatible export surface.
def_file_link_flags = (["/DEF:" + def_file.path] if is_windows else
["@" + def_file.path])
#
# A module-definition file has no counterpart in the GNU toolchain; a
# version script (`-Wl,--version-script=`) is the equivalent there and
# is a different file format, so a `.def` cannot be forwarded.
if not is_windows:
fail("'{}': def_file is supported on Windows only".format(name))
def_file_link_flags = ["/DEF:" + def_file.path]
linking_outputs = cc_common.link(
name = name,
actions = actions,
Expand Down
48 changes: 33 additions & 15 deletions dev/bazel/daal.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,6 @@ daal_generate_version = rule(
},
)

def _get_tool_for_kernel_defines_patching(ctx):
return ctx.toolchains["@onedal//dev/bazel/toolchains:extra"] \
.extra_toolchain_info.patch_daal_kernel_defines

def _get_disabled_cpus(ctx):
cpu_info = ctx.attr._cpus[CpuInfo]
all_cpus = sets.make(cpu_info.allowed)
Expand All @@ -178,17 +174,37 @@ def _declare_patched_kernel_defines(ctx):
return ctx.actions.declare_file(patched_path)

def _daal_patch_kernel_defines_impl(ctx):
disabled_cpus = _get_disabled_cpus(ctx)
"""Comment out the `DAAL_KERNEL_<ISA>` defines of the disabled ISAs.

`cpp/daal/include/services/internal/daal_kernel_defines.h` enables every
CPU dispatch variant oneDAL can build; the ones `--cpu` leaves out have to
be removed from the header before it is compiled or released. The Makefile
does it with

sed -b -i -E -e 's/^#define DAAL_KERNEL_<ISA>\\b/$(sed.eol)/'

where `sed.eol` is empty on Linux and a lone CR on Windows, so that a
disabled define leaves behind an empty line with the platform's own line
ending while every other line is untouched. `expand_template` does exactly
that substitution, without a helper script: it rewrites the file at
execution time, so nothing has to read its contents during analysis.

The substitution is literal, not anchored like sed's `^...\\b`, which is
equivalent here because the header holds one bare `#define DAAL_KERNEL_*`
per line and mentions those macros nowhere else.
"""
disabled_cpus = sets.to_list(_get_disabled_cpus(ctx))
kernel_defines = _declare_patched_kernel_defines(ctx)
ctx.actions.run(
executable = _get_tool_for_kernel_defines_patching(ctx),
arguments = [
ctx.file.src.path,
kernel_defines.path,
" ".join(sets.to_list(disabled_cpus)),
],
inputs = [ctx.file.src],
outputs = [kernel_defines],
is_windows = ctx.target_platform_has_constraint(
ctx.attr._windows_constraint[platform_common.ConstraintValueInfo],
)
ctx.actions.expand_template(
template = ctx.file.src,
output = kernel_defines,
substitutions = {
"#define DAAL_KERNEL_{}".format(cpu.upper()): "\r" if is_windows else ""
for cpu in disabled_cpus
},
)
return [ DefaultInfo(files=depset([ kernel_defines ])) ]

Expand All @@ -200,6 +216,8 @@ daal_patch_kernel_defines = rule(
"_cpus": attr.label(
default = "@config//:cpu",
),
"_windows_constraint": attr.label(
default = "@platforms//os:windows",
),
},
toolchains = ["@onedal//dev/bazel/toolchains:extra"],
)
4 changes: 0 additions & 4 deletions dev/bazel/release.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,6 @@ def _copy_data(ctx, prefix):
return dst_files

def _copy_to_release_impl(ctx):
extra_toolchain = ctx.toolchains["@onedal//dev/bazel/toolchains:extra"]
prefix = ctx.attr.name + "/daal/latest"
version_info = ctx.attr._version_info[VersionInfo] if ctx.attr._version_info else None
files = []
Expand Down Expand Up @@ -641,9 +640,6 @@ _release = rule(
default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
),
},
toolchains = [
"@onedal//dev/bazel/toolchains:extra"
],
)

def _headers_filter_impl(ctx):
Expand Down
2 changes: 0 additions & 2 deletions dev/bazel/toolchains/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,3 @@
#===============================================================================

package(default_visibility = ["//visibility:public"])

toolchain_type(name = "extra")
21 changes: 0 additions & 21 deletions dev/bazel/toolchains/action_names.bzl

This file was deleted.

Loading
Loading