Cleanup FDO configs
All changes are a no-op.
Base conditions on FdoContext instead of duplicating it from C++ configuration. The latter is error prone.
Remove unused cppConfiguration from hasArtifact method.
Change absolute paths in C++ configuration from PathFragments to Strings. Starlarkified code only needs String (PathFragments are not used in Starlark).
Remove unused methods from CppConfiguration.
PiperOrigin-RevId: 626366966
Change-Id: I767771611374e02e58b1f34a203311ff2ebc5bdb
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
index b227d7a..6ef2ca0 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
@@ -471,9 +471,10 @@
FdoContext.BranchFdoProfile branchFdoProvider = toolchain.getFdoContext().getBranchFdoProfile();
boolean enablePropellerOptimize =
- (cppConfiguration.getPropellerOptimizeLabel() != null
- || cppConfiguration.getPropellerOptimizeAbsoluteCCProfile() != null
- || cppConfiguration.getPropellerOptimizeAbsoluteLdProfile() != null);
+ (toolchain.getFdoContext().getPropellerOptimizeInputFile() != null
+ && (toolchain.getFdoContext().getPropellerOptimizeInputFile().getCcArtifact() != null
+ || toolchain.getFdoContext().getPropellerOptimizeInputFile().getLdArtifact()
+ != null));
if (branchFdoProvider != null && cppConfiguration.getCompilationMode() == CompilationMode.OPT) {
if ((branchFdoProvider.isLlvmFdo() || branchFdoProvider.isLlvmCSFdo())
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java
index ab0d810..fc941a8 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java
@@ -93,7 +93,7 @@
// FDO is disabled -> do nothing.
Preconditions.checkNotNull(fdoContext);
- if (!fdoContext.hasArtifacts(cppConfiguration)) {
+ if (!fdoContext.hasArtifacts()) {
return;
}
@@ -1277,7 +1277,7 @@
if (builder.getDiagnosticsFile() != null) {
diagnosticsFileExecPath = builder.getDiagnosticsFile().getExecPathString();
}
- if (needsFdoBuildVariables && fdoContext.hasArtifacts(cppConfiguration)) {
+ if (needsFdoBuildVariables && fdoContext.hasArtifacts()) {
// This modifies the passed-in builder, which is a surprising side-effect, and makes it unsafe
// to call this method multiple times for the same builder.
builder.addMandatoryInputs(getAuxiliaryFdoInputs());
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
index f352d97..941cd2e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -146,12 +146,12 @@
*/
public static final String FDO_STAMP_MACRO = "BUILD_FDO_TYPE";
- private final PathFragment fdoPath;
+ private final String fdoPath;
private final Label fdoOptimizeLabel;
- private final PathFragment csFdoAbsolutePath;
- private final PathFragment propellerOptimizeAbsoluteCCProfile;
- private final PathFragment propellerOptimizeAbsoluteLdProfile;
+ private final String csFdoAbsolutePath;
+ private final String propellerOptimizeAbsoluteCCProfile;
+ private final String propellerOptimizeAbsoluteLdProfile;
private final ImmutableList<String> conlyopts;
@@ -261,11 +261,17 @@
}
}
- this.fdoPath = fdoPath;
+ this.fdoPath = fdoPath == null ? null : fdoPath.getPathString();
this.fdoOptimizeLabel = fdoProfileLabel;
- this.csFdoAbsolutePath = csFdoAbsolutePath;
- this.propellerOptimizeAbsoluteCCProfile = propellerOptimizeAbsoluteCCProfile;
- this.propellerOptimizeAbsoluteLdProfile = propellerOptimizeAbsoluteLdProfile;
+ this.csFdoAbsolutePath = csFdoAbsolutePath == null ? null : csFdoAbsolutePath.getPathString();
+ this.propellerOptimizeAbsoluteCCProfile =
+ propellerOptimizeAbsoluteCCProfile == null
+ ? null
+ : propellerOptimizeAbsoluteCCProfile.getPathString();
+ this.propellerOptimizeAbsoluteLdProfile =
+ propellerOptimizeAbsoluteLdProfile == null
+ ? null
+ : propellerOptimizeAbsoluteLdProfile.getPathString();
this.conlyopts = ImmutableList.copyOf(cppOptions.conlyoptList);
this.copts = ImmutableList.copyOf(cppOptions.coptList);
this.cxxopts = ImmutableList.copyOf(cppOptions.cxxoptList);
@@ -300,7 +306,7 @@
public Label getFdoZipper() {
if (getFdoOptimizeLabel() != null
|| getFdoProfileLabel() != null
- || getFdoPath() != null
+ || fdoPath != null
|| getMemProfProfileLabel() != null) {
return Label.parseCanonicalUnchecked(BAZEL_TOOLS_REPO + "//tools/zip:unzip_fdo");
}
@@ -353,14 +359,6 @@
return cppOptions.dynamicMode.name();
}
- public boolean isFdo() {
- return cppOptions.isFdo();
- }
-
- public boolean isCSFdo() {
- return cppOptions.isCSFdo();
- }
-
public boolean useArgsParamsFile() {
return cppOptions.useArgsParamsFile;
}
@@ -604,10 +602,6 @@
return cppOptions.fdoInstrumentForBuild;
}
- PathFragment getFdoPath() {
- return fdoPath;
- }
-
@StarlarkMethod(
name = "fdo_path",
documented = false,
@@ -628,10 +622,6 @@
return cppOptions.csFdoInstrumentForBuild;
}
- public PathFragment getCSFdoAbsolutePath() {
- return csFdoAbsolutePath;
- }
-
@StarlarkMethod(
name = "cs_fdo_path",
documented = false,
@@ -640,11 +630,7 @@
@Nullable
public String getCsFdoPathForStarlark(StarlarkThread thread) throws EvalException {
CcModule.checkPrivateStarlarkificationAllowlist(thread);
- return csFdoAbsolutePath == null ? null : csFdoAbsolutePath.toString();
- }
-
- public PathFragment getPropellerOptimizeAbsoluteCCProfile() {
- return propellerOptimizeAbsoluteCCProfile;
+ return csFdoAbsolutePath;
}
@StarlarkMethod(
@@ -656,14 +642,7 @@
public String getPropellerOptimizeAbsoluteCcProfileForStarlark(StarlarkThread thread)
throws EvalException {
CcModule.checkPrivateStarlarkificationAllowlist(thread);
- if (getPropellerOptimizeAbsoluteCCProfile() == null) {
- return null;
- }
- return getPropellerOptimizeAbsoluteCCProfile().toString();
- }
-
- public PathFragment getPropellerOptimizeAbsoluteLdProfile() {
- return propellerOptimizeAbsoluteLdProfile;
+ return propellerOptimizeAbsoluteCCProfile;
}
@StarlarkMethod(
@@ -675,10 +654,7 @@
public String getPropellerOptimizeAbsoluteLdProfileForStarlark(StarlarkThread thread)
throws EvalException {
CcModule.checkPrivateStarlarkificationAllowlist(thread);
- if (getPropellerOptimizeAbsoluteLdProfile() == null) {
- return null;
- }
- return getPropellerOptimizeAbsoluteLdProfile().toString();
+ return propellerOptimizeAbsoluteLdProfile;
}
@Nullable
@@ -733,10 +709,6 @@
return cppOptions.getMemProfProfileLabel();
}
- public boolean isFdoAbsolutePathEnabled() {
- return cppOptions.enableFdoProfileAbsolutePath;
- }
-
@StarlarkMethod(
name = "enable_fdo_profile_absolute_path",
documented = false,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
index 6989e2f..2a264e7 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
@@ -262,10 +262,12 @@
return featureConfiguration.isEnabled(CppRuleClasses.XBINARYFDO) ? "XFDO" : null;
}
}
- if (cppConfiguration.isCSFdo()) {
+ if (fdoContext.getBranchFdoProfile() != null
+ && (fdoContext.getBranchFdoProfile().isLlvmCSFdo()
+ || cppConfiguration.getCSFdoInstrument() != null)) {
return "CSFDO";
}
- if (cppConfiguration.isFdo()) {
+ if (fdoContext.getBranchFdoProfile() != null || cppConfiguration.getFdoInstrument() != null) {
return "FDO";
}
return null;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
index 0cccd2e..c731f91 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
@@ -1066,17 +1066,4 @@
}
return this;
}
-
- /** Returns true if targets under this configuration should apply FDO. */
- public boolean isFdo() {
- return getFdoOptimize() != null || fdoInstrumentForBuild != null || fdoProfileLabel != null;
- }
-
- /** Returns true if targets under this configuration should apply CSFdo. */
- public boolean isCSFdo() {
- return (getFdoOptimize() != null || fdoProfileLabel != null)
- && (csFdoInstrumentForBuild != null
- || csFdoProfileLabel != null
- || csFdoAbsolutePathForBuild != null);
- }
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoContext.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoContext.java
index 5270106..f40d9dd 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoContext.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoContext.java
@@ -103,7 +103,7 @@
return fdoContextStruct.getNoneableValue("memprof_profile_artifact", Artifact.class);
}
- boolean hasArtifacts(CppConfiguration cppConfiguration) throws EvalException {
+ boolean hasArtifacts() throws EvalException {
return getBranchFdoProfile() != null
|| getPrefetchHintsArtifact() != null
|| getPropellerOptimizeInputFile() != null
diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_toolchain_provider_helper.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_toolchain_provider_helper.bzl
index 9460e27..fa7188e 100644
--- a/src/main/starlark/builtins_bzl/common/cc/cc_toolchain_provider_helper.bzl
+++ b/src/main/starlark/builtins_bzl/common/cc/cc_toolchain_provider_helper.bzl
@@ -185,9 +185,6 @@
toolchain_features = cc_internal.cc_toolchain_features(toolchain_config_info = toolchain_config_info, tools_directory = tools_directory)
fdo_context = create_fdo_context(
ctx = ctx,
- configuration = ctx.configuration,
- cpp_config = ctx.fragments.cpp,
- tool_paths = tool_paths,
fdo_prefetch_provider = attributes.fdo_prefetch_provider,
propeller_optimize_provider = attributes.propeller_optimize_provider,
mem_prof_profile_provider = attributes.mem_prof_profile_provider,
@@ -195,6 +192,7 @@
fdo_profile_provider = attributes.fdo_profile_provider,
x_fdo_profile_provider = attributes.x_fdo_profile_provider,
cs_fdo_profile_provider = attributes.cs_fdo_profile_provider,
+ llvm_profdata = tool_paths.get("llvm-profdata"),
all_files = attributes.all_files,
zipper = attributes.zipper,
cc_toolchain_config_info = attributes.cc_toolchain_config_info,
diff --git a/src/main/starlark/builtins_bzl/common/cc/fdo/fdo_context.bzl b/src/main/starlark/builtins_bzl/common/cc/fdo/fdo_context.bzl
index 62e5ada..9be5afb 100644
--- a/src/main/starlark/builtins_bzl/common/cc/fdo/fdo_context.bzl
+++ b/src/main/starlark/builtins_bzl/common/cc/fdo/fdo_context.bzl
@@ -23,9 +23,6 @@
def create_fdo_context(
*,
ctx,
- configuration,
- cpp_config,
- tool_paths,
fdo_prefetch_provider,
propeller_optimize_provider,
mem_prof_profile_provider,
@@ -33,12 +30,43 @@
fdo_profile_provider,
x_fdo_profile_provider,
cs_fdo_profile_provider,
+ llvm_profdata,
all_files,
zipper,
cc_toolchain_config_info,
fdo_optimize_artifacts,
fdo_optimize_label):
- """Creates FDO context."""
+ """Creates FDO context when it should be available.
+
+ When `-c opt` is used it parses values of FDO related flags, processes the
+ input Files and return Files ready to be used for FDO.
+
+ Args:
+ ctx: (RuleContext) used to create actions and obtain configuration
+ fdo_prefetch_provider: (FdoPrefetchHintsInfo) Pointed to by --fdo_prefetch_hints
+ propeller_optimize_provider: (PropellerOptimizeInfo) Pointed to by --propeller_optimize
+ mem_prof_profile_provider: (MemProfProfileInfo) Pointed to by --memprof_profile
+ fdo_optimize_provider: (PropellerOptimizeInfo) Pointed to by --propeller_optimize
+ fdo_profile_provider: (FdoProfileInfo) Pointed to by --fdo_profile
+ x_fdo_profile_provider: (FdoProfileInfo) Pointed to by --xbinary_fdo
+ cs_fdo_profile_provider: (FdoProfileInfo) Pointed to by --cs_fdo_profile
+ llvm_profdata: (File) llvm-profdata executable
+ all_files: (depset(File)) Files needed to run llvm-profdata
+ zipper: (File) zip tool, used to unpact the profiles
+ cc_toolchain_config_info: (CcToolchainConfigInfo) Used to check CPU value, should be removed
+ fdo_optimize_artifacts: (list[File]) If any a list of files pointed to by --fdo_profile
+ fdo_optimize_label: (Label) If any a label of target pointed to by --fdo_profile
+ Returns:
+ (FDOContext) A structure with following fields:
+ - branch_fdo_profile (struct|None)
+ - branch_fdo_mode ("auto_fdo"|"xbinary_fdo"|"llvm_fdo"|llvm_cs_fdo")
+ - profile_artifact (File)
+ - proto_profile_artifact (File)
+ - prefetch_hints_artifact (File),
+ - propeller_optimize_info (PropellerOptimizeInfo)
+ - memprof_profile_artifact (File)
+ """
+ cpp_config = ctx.fragments.cpp
if cpp_config.compilation_mode() != "opt":
return struct()
@@ -122,7 +150,7 @@
if x_fdo_profile_provider:
fail("--xbinary_fdo only accepts *.xfdo and *.afdo")
- if configuration.coverage_enabled:
+ if ctx.configuration.coverage_enabled:
fail("coverage mode is not compatible with FDO optimization")
# This tries to convert LLVM profiles to the indexed format if necessary.
@@ -131,7 +159,7 @@
ctx,
"fdo",
fdo_inputs,
- tool_paths,
+ llvm_profdata,
all_files,
zipper,
cc_toolchain_config_info,
@@ -148,7 +176,7 @@
ctx,
"fdo",
fdo_inputs,
- tool_paths,
+ llvm_profdata,
all_files,
zipper,
cc_toolchain_config_info,
@@ -157,7 +185,7 @@
ctx,
"csfdo",
cs_fdo_input,
- tool_paths,
+ llvm_profdata,
all_files,
zipper,
cc_toolchain_config_info,
@@ -165,7 +193,7 @@
profile_artifact = _merge_llvm_profiles(
ctx,
"mergedfdo",
- tool_paths,
+ llvm_profdata,
all_files,
non_cs_profile_artifact,
cs_profile_artifact,
@@ -198,7 +226,7 @@
ctx,
name_prefix,
fdo_inputs,
- tool_paths,
+ llvm_profdata,
all_files,
zipper,
cc_toolchain_config_info):
@@ -246,14 +274,14 @@
else: # .profraw
raw_profile_artifact = _symlink_input(ctx, name_prefix, fdo_inputs, "Symlinking LLVM Raw Profile %{input}")
- if not tool_paths.get("llvm-profdata"):
+ if not llvm_profdata:
fail("llvm-profdata not available with this crosstool, needed for profile conversion")
name = name_prefix + "/" + ctx.label.name + "/" + paths.replace_extension(basename, ".profdata")
profile_artifact = ctx.actions.declare_file(name)
ctx.actions.run(
mnemonic = "LLVMProfDataAction",
- executable = tool_paths.get("llvm-profdata"),
+ executable = llvm_profdata,
tools = [all_files],
arguments = [ctx.actions.args().add("merge").add("-o").add(profile_artifact).add(raw_profile_artifact)],
inputs = [raw_profile_artifact],
@@ -268,7 +296,7 @@
def _merge_llvm_profiles(
ctx,
name_prefix,
- tool_paths,
+ llvm_profdata,
all_files,
profile1,
profile2,
@@ -279,7 +307,7 @@
# Merge LLVM profiles.
ctx.actions.run(
mnemonic = "LLVMProfDataMergeAction",
- executable = tool_paths.get("llvm-profdata"),
+ executable = llvm_profdata,
tools = [all_files],
arguments = [ctx.actions.args().add("merge").add("-o").add(profile_artifact).add(profile1).add(profile2)],
inputs = [profile1, profile2],
