Skip to content

Commit

Permalink
Pass FDO options to the host configuration, but only enable FDO for …
Browse files Browse the repository at this point in the history
…target configuration

    This is there to allow cc_toolchain to detect that FDO is active, and to
    preprocess profiles, while being analyzed in the host configuration. FDO related
    options are passed to the host configuration, they are preprocessed in the host
    configuration, but then used in the target configuration. Extra care is made to
    not enable FDO in the host configuration (as this is the existing behavior).

    bazelbuild/bazel#6516

    RELNOTES: None
    PiperOrigin-RevId: 240721577
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent 2cc9928 commit 3bddb34
Show file tree
Hide file tree
Showing 6 changed files with 34 additions and 43 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1443,8 +1443,6 @@ private CcToolchainVariables setupCompileBuildVariables(
ruleErrorConsumer,
featureConfiguration,
ccToolchain,
configuration.getOptions(),
cppConfiguration,
toPathString(sourceFile),
toPathString(builder.getOutputFile()),
toPathString(gcnoFile),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ rules_cc related actions (with the exception of actions that are using more prec
.add(
attr("all_files", LABEL)
.legacyAllowAnyFileType()
.cfg(HostTransition.createFactory())
.cfg(HostTransition.INSTANCE)
.mandatory())
/* <!-- #BLAZE_RULE(cc_toolchain).ATTRIBUTE(compiler_files) -->
Collection of all cc_toolchain artifacts required for compile actions.
Expand All @@ -173,7 +173,7 @@ rules_cc related actions (with the exception of actions that are using more prec
.add(
attr("compiler_files", LABEL)
.legacyAllowAnyFileType()
.cfg(HostTransition.createFactory())
.cfg(HostTransition.INSTANCE)
.mandatory())
/* <!-- #BLAZE_RULE(cc_toolchain).ATTRIBUTE(compiler_files_without_includes) -->
Collection of all cc_toolchain artifacts required for compile actions in case when
Expand All @@ -182,35 +182,35 @@ input discovery is supported (currently Google-only).
.add(
attr("compiler_files_without_includes", LABEL)
.legacyAllowAnyFileType()
.cfg(HostTransition.createFactory()))
.cfg(HostTransition.INSTANCE))
/* <!-- #BLAZE_RULE(cc_toolchain).ATTRIBUTE(strip_files) -->
Collection of all cc_toolchain artifacts required for strip actions.
<!-- #END_BLAZE_RULE.ATTRIBUTE -->*/
.add(
attr("strip_files", LABEL)
.legacyAllowAnyFileType()
.cfg(HostTransition.createFactory())
.cfg(HostTransition.INSTANCE)
.mandatory())
/* <!-- #BLAZE_RULE(cc_toolchain).ATTRIBUTE(objcopy_files) -->
Collection of all cc_toolchain artifacts required for objcopy actions.
<!-- #END_BLAZE_RULE.ATTRIBUTE -->*/
.add(
attr("objcopy_files", LABEL)
.legacyAllowAnyFileType()
.cfg(HostTransition.createFactory())
.cfg(HostTransition.INSTANCE)
.mandatory())
/* <!-- #BLAZE_RULE(cc_toolchain).ATTRIBUTE(as_files) -->
Currently unused (<a href="https://github.com/bazelbuild/bazel/issues/6928">#6928</a>).
<p>Collection of all cc_toolchain artifacts required for assembly actions.</p>
<!-- #END_BLAZE_RULE.ATTRIBUTE -->*/
.add(attr("as_files", LABEL).legacyAllowAnyFileType().cfg(HostTransition.createFactory()))
.add(attr("as_files", LABEL).legacyAllowAnyFileType().cfg(HostTransition.INSTANCE))
/* <!-- #BLAZE_RULE(cc_toolchain).ATTRIBUTE(as_files) -->
Currently unused (<a href="https://github.com/bazelbuild/bazel/issues/6928">#6928</a>).
<p>Collection of all cc_toolchain artifacts required for archiving actions.</p>
<!-- #END_BLAZE_RULE.ATTRIBUTE -->*/
.add(attr("ar_files", LABEL).legacyAllowAnyFileType().cfg(HostTransition.createFactory()))
.add(attr("ar_files", LABEL).legacyAllowAnyFileType().cfg(HostTransition.INSTANCE))
/* <!-- #BLAZE_RULE(cc_toolchain).ATTRIBUTE(linker_files) -->
Collection of all cc_toolchain artifacts required for linking actions.
Expand All @@ -220,24 +220,21 @@ Currently unused (<a href="https://github.com/bazelbuild/bazel/issues/6928">#692
.add(
attr("linker_files", LABEL)
.legacyAllowAnyFileType()
.cfg(HostTransition.createFactory())
.cfg(HostTransition.INSTANCE)
.mandatory())
/* <!-- #BLAZE_RULE(cc_toolchain).ATTRIBUTE(dwp_files) -->
Collection of all cc_toolchain artifacts required for dwp actions.
<!-- #END_BLAZE_RULE.ATTRIBUTE -->*/
.add(
attr("dwp_files", LABEL)
.legacyAllowAnyFileType()
.cfg(HostTransition.createFactory())
.cfg(HostTransition.INSTANCE)
.mandatory())
/* <!-- #BLAZE_RULE(cc_toolchain).ATTRIBUTE(coverage_files) -->
Collection of all cc_toolchain artifacts required for coverage actions. If not specified,
all_files are used.
<!-- #END_BLAZE_RULE.ATTRIBUTE -->*/
.add(
attr("coverage_files", LABEL)
.legacyAllowAnyFileType()
.cfg(HostTransition.createFactory()))
.add(attr("coverage_files", LABEL).legacyAllowAnyFileType().cfg(HostTransition.INSTANCE))
/* <!-- #BLAZE_RULE(cc_toolchain).ATTRIBUTE(static_runtime_lib) -->
Static library artifact for the C++ runtime library (e.g. libstdc++.a).
Expand All @@ -255,7 +252,7 @@ Currently unused (<a href="https://github.com/bazelbuild/bazel/issues/6928">#692
/* <!-- #BLAZE_RULE(cc_toolchain).ATTRIBUTE(module_map) -->
Module map artifact to be used for modular builds.
<!-- #END_BLAZE_RULE.ATTRIBUTE -->*/
.add(attr("module_map", LABEL).legacyAllowAnyFileType().cfg(HostTransition.createFactory()))
.add(attr("module_map", LABEL).legacyAllowAnyFileType().cfg(HostTransition.INSTANCE))
/* <!-- #BLAZE_RULE(cc_toolchain).ATTRIBUTE(supports_param_files) -->
Set to True when cc_toolchain supports using param files for linking actions.
<!-- #END_BLAZE_RULE.ATTRIBUTE -->*/
Expand All @@ -266,20 +263,20 @@ Currently unused (<a href="https://github.com/bazelbuild/bazel/issues/6928">#692
.add(attr("supports_header_parsing", BOOLEAN).value(false))
.add(
attr("$interface_library_builder", LABEL)
.cfg(HostTransition.createFactory())
.cfg(HostTransition.INSTANCE)
.singleArtifact()
.value(env.getToolsLabel("//tools/cpp:interface_library_builder")))
.add(
attr("$link_dynamic_library_tool", LABEL)
.cfg(HostTransition.createFactory())
.cfg(HostTransition.INSTANCE)
.singleArtifact()
.value(env.getToolsLabel("//tools/cpp:link_dynamic_library")))
.add(
attr(CcToolchain.CC_TOOLCHAIN_TYPE_ATTRIBUTE_NAME, NODEP_LABEL)
.value(CppRuleClasses.ccToolchainTypeAttribute(env)))
.add(
attr(":zipper", LABEL)
.cfg(HostTransition.createFactory())
.cfg(HostTransition.INSTANCE)
.singleArtifact()
.value(
LabelLateBoundDefault.fromTargetConfiguration(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -421,14 +421,6 @@ public boolean isOmitfp() {
/** Returns flags passed to Bazel by --copt option. */
@Override
public ImmutableList<String> getCopts() {
if (isOmitfp()) {
return ImmutableList.<String>builder()
.add("-fomit-frame-pointer")
.add("-fasynchronous-unwind-tables")
.add("-DNO_FRAME_POINTER")
.addAll(copts)
.build();
}
return copts;
}

Expand Down Expand Up @@ -590,6 +582,22 @@ public boolean useLLVMCoverageMapFormat() {
return cppOptions.useLLVMCoverageMapFormat;
}

public boolean disableLegacyCrosstoolFields() {
return cppOptions.disableLegacyCrosstoolFields;
}

public boolean disableExpandIfAllAvailableInFlagSet() {
return cppOptions.disableExpandIfAllAvailableInFlagSet;
}

public static String getLegacyCrosstoolFieldErrorMessage(String field) {
Preconditions.checkNotNull(field);
return field
+ " is disabled by --incompatible_disable_legacy_crosstool_fields, please "
+ "migrate your CROSSTOOL (see https://github.com/bazelbuild/bazel/issues/6861 for "
+ "migration instructions).";
}

public boolean removeCpuCompilerCcToolchainAttributes() {
return cppOptions.removeCpuCompilerCcToolchainAttributes;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -781,7 +781,7 @@ public Label getFdoPrefetchHintsLabel() {

@Option(
name = "incompatible_disable_crosstool_file",
defaultValue = "true",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
metadataTags = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,6 @@ public enum BranchFdoMode {

/** Instrumentation-based FDO implemented on LLVM. */
LLVM_FDO,

/** Instrumentation-based Context Sensitive FDO implemented on LLVM. */
LLVM_CS_FDO,
}

/** A POJO encapsulating the branch profiling configuration. */
Expand Down Expand Up @@ -71,10 +68,6 @@ public boolean isLlvmFdo() {
return branchFdoMode == BranchFdoMode.LLVM_FDO;
}

public boolean isLlvmCSFdo() {
return branchFdoMode == BranchFdoMode.LLVM_CS_FDO;
}

public Artifact getProfileArtifact() {
return profileArtifact;
}
Expand Down Expand Up @@ -102,7 +95,7 @@ public Artifact getPrefetchHintsArtifact() {
}

boolean hasArtifacts(CppConfiguration cppConfiguration) {
if (cppConfiguration.isToolConfigurationDoNotUseWillBeRemovedFor129045294()) {
if (cppConfiguration.isThisHostConfigurationDoNotUseWillBeRemovedFor129045294()) {
// We don't want FDO for host configuration
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,7 @@ public static FdoContext getFdoContext(
Artifact profileArtifact = null;
if (branchFdoMode == BranchFdoMode.LLVM_FDO) {
profileArtifact =
convertLLVMRawProfileToIndexed(
attributes, fdoInputFile, toolchainInfo, ruleContext, cppConfiguration);
convertLLVMRawProfileToIndexed(attributes, fdoInputFile, toolchainInfo, ruleContext);
if (ruleContext.hasErrors()) {
return null;
}
Expand Down Expand Up @@ -217,11 +216,7 @@ private static Artifact convertLLVMRawProfileToIndexed(
CcToolchainAttributesProvider attributes,
FdoInputFile fdoProfile,
CppToolchainInfo toolchainInfo,
RuleContext ruleContext,
CppConfiguration cppConfiguration) {
if (cppConfiguration.isThisHostConfigurationDoNotUseWillBeRemovedFor129045294()) {
return null;
}
RuleContext ruleContext) {

Artifact profileArtifact =
ruleContext.getUniqueDirectoryArtifact(
Expand Down

0 comments on commit 3bddb34

Please sign in to comment.