Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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
90 changes: 52 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,74 @@ 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 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, and loose object
files can go in alongside the archives. 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. On Linux `static_libs` holds archives only, since
object files go through `cc_common` first.

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: static libraries to merge, plus loose object files on
Windows.
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 +307,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