Skip to content

Commit

Permalink
Don't propagate FDO to exec actions.
Browse files Browse the repository at this point in the history 
Benefits:

 - More efficient caching for exec actions (which shouldn't integrate FDO data)
 - simpler, more straightforward code
 - resolves outdated logic from before toolchain transitions (https://github.com/bazelbuild/proposals/blob/main/designs/2020-02-07-toolchain-transition-migration.md)

#6516

PiperOrigin-RevId: 478011721
Change-Id: I8df049eeb78003fa9e979a91313e45cb9de95d01
  • Loading branch information
@gregestren
gregestren authored and Copybara-Service committed Sep 30, 2022
1 parent 6fccee7 commit 56dba00
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 118 deletions.
26 changes: 6 additions & 20 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainRule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,15 +94,13 @@ private static Label getLabel(AttributeMap attributes, String attrName, Label de
LabelLateBoundDefault.fromTargetConfiguration(
CppConfiguration.class,
null,
(rule, attributes, cppConfig) ->
cppConfig.getFdoOptimizeLabelUnsafeSinceItCanReturnValueFromWrongConfiguration());
(rule, attributes, cppConfig) -> cppConfig.getFdoOptimizeLabel());

private static final LabelLateBoundDefault<?> FDO_PROFILE_VALUE =
LabelLateBoundDefault.fromTargetConfiguration(
CppConfiguration.class,
null,
(rule, attributes, cppConfig) ->
cppConfig.getFdoProfileLabelUnsafeSinceItCanReturnValueFromWrongConfiguration());
(rule, attributes, cppConfig) -> cppConfig.getFdoProfileLabel());

private static final LabelLateBoundDefault<?> CSFDO_PROFILE_VALUE =
LabelLateBoundDefault.fromTargetConfiguration(
Expand All @@ -114,8 +112,7 @@ private static Label getLabel(AttributeMap attributes, String attrName, Label de
LabelLateBoundDefault.fromTargetConfiguration(
CppConfiguration.class,
null,
(rule, attributes, cppConfig) ->
cppConfig.getXFdoProfileLabelUnsafeSinceItCanReturnValueFromWrongConfiguration());
(rule, attributes, cppConfig) -> cppConfig.getXFdoProfileLabel());

private static final LabelLateBoundDefault<?> FDO_PREFETCH_HINTS =
LabelLateBoundDefault.fromTargetConfiguration(
Expand All @@ -134,20 +131,9 @@ private static Label getLabel(AttributeMap attributes, String attrName, Label de
* enabled through --fdo_optimize or --fdo_profile
*/
private static boolean shouldIncludeZipperInToolchain(CppConfiguration cppConfiguration) {
if (cppConfiguration.isToolConfigurationDoNotUseWillBeRemovedFor129045294()) {
// This is actually a bug, because with platforms, and before toolchain-transitions are
// implemented, all toolchains are analyzed in the host configuration. We're betting on
// nobody in the Bazel world actually using zipped fdo profiles and platforms at the same
// time, and if people do, they'll have to wait for toolchain-transitions. This needs to be
// fixed.
// TODO(b/129045294): Fix this once toolchain-transitions are implemented.
return false;
}
return cppConfiguration.getFdoOptimizeLabelUnsafeSinceItCanReturnValueFromWrongConfiguration()
!= null
|| cppConfiguration.getFdoProfileLabelUnsafeSinceItCanReturnValueFromWrongConfiguration()
!= null
|| cppConfiguration.getFdoPathUnsafeSinceItCanReturnValueFromWrongConfiguration() != null;
return cppConfiguration.getFdoOptimizeLabel() != null
|| cppConfiguration.getFdoProfileLabel() != null
|| cppConfiguration.getFdoPath() != null;
}

@Override
Expand Down
66 changes: 7 additions & 59 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -620,28 +620,14 @@ public boolean isStrictSystemIncludes() {

@Nullable
String getFdoInstrument() {
if (isToolConfigurationDoNotUseWillBeRemovedFor129045294()) {
// We don't want FDO in the host configuration
return null;
}
return cppOptions.fdoInstrumentForBuild;
}

/**
* @deprecated Unsafe because it returns a value from target configuration even in the host
* configuration.
*/
@Deprecated
PathFragment getFdoPathUnsafeSinceItCanReturnValueFromWrongConfiguration() {
PathFragment getFdoPath() {
return fdoPath;
}

/**
* @deprecated Unsafe because it returns a value from target configuration even in the host
* configuration.
*/
@Deprecated
Label getFdoOptimizeLabelUnsafeSinceItCanReturnValueFromWrongConfiguration() {
Label getFdoOptimizeLabel() {
return fdoOptimizeLabel;
}

Expand All @@ -663,59 +649,27 @@ public PathFragment getPropellerOptimizeAbsoluteLdProfile() {

@Nullable
Label getFdoPrefetchHintsLabel() {
if (isToolConfigurationDoNotUseWillBeRemovedFor129045294()) {
// We don't want FDO in the host configuration
return null;
}
return getFdoPrefetchHintsLabelUnsafeSinceItCanReturnValueFromWrongConfiguration();
}

/**
* @deprecated Unsafe because it returns a value from target configuration even in the host
* configuration.
*/
@Deprecated
Label getFdoPrefetchHintsLabelUnsafeSinceItCanReturnValueFromWrongConfiguration() {
return cppOptions.getFdoPrefetchHintsLabel();
}

/**
* @deprecated Unsafe because it returns a value from target configuration even in the host
* configuration.
*/
@Deprecated
Label getFdoProfileLabelUnsafeSinceItCanReturnValueFromWrongConfiguration() {
Label getFdoProfileLabel() {
return cppOptions.fdoProfileLabel;
}

public Label getCSFdoProfileLabel() {
return cppOptions.csFdoProfileLabel;
}

public Label getPropellerOptimizeLabel() {
return cppOptions.propellerOptimizeLabel;
}

/**
* @deprecated Unsafe because it returns a value from target configuration even in the host
* configuration.
*/
@Nullable
@Deprecated
Label getPropellerOptimizeLabelUnsafeSinceItCanReturnValueFromWrongConfiguration() {
Label getPropellerOptimizeLabel() {
if (cppOptions.fdoInstrumentForBuild != null || cppOptions.csFdoInstrumentForBuild != null) {
return null;
}
return cppOptions.getPropellerOptimizeLabel();
}

/**
* @deprecated Unsafe because it returns a value from target configuration even in the host
* configuration.
*/
@Nullable
@Deprecated
Label getXFdoProfileLabelUnsafeSinceItCanReturnValueFromWrongConfiguration() {
Label getXFdoProfileLabel() {
if (cppOptions.fdoOptimizeForBuild != null
|| cppOptions.fdoInstrumentForBuild != null
|| cppOptions.fdoProfileLabel != null
Expand Down Expand Up @@ -770,7 +724,7 @@ public boolean shareNativeDepsStarlark(StarlarkThread thread) throws EvalExcepti
*/
@Nullable
public Label getTargetLibcTopLabel() {
if (!isToolConfigurationDoNotUseWillBeRemovedFor129045294()) {
if (!isToolConfigurationDoNotUseWillBeRemovedFor129045294) {
// This isn't for a platform-enabled C++ toolchain (legacy C++ toolchains evaluate in the
// target configuration while platform-enabled toolchains evaluate in the host/exec
// configuration). targetLibcTopLabel is only intended for platform-enabled toolchains and can
Expand Down Expand Up @@ -807,13 +761,6 @@ public boolean collectCodeCoverage() {
return collectCodeCoverage;
}

/** @deprecated this is only a temporary workaround, will be removed by b/129045294. */
// TODO(b/129045294): Remove at first opportunity
@Deprecated
boolean isToolConfigurationDoNotUseWillBeRemovedFor129045294() {
return isToolConfigurationDoNotUseWillBeRemovedFor129045294;
}

public boolean enableCcToolchainResolution() {
return cppOptions.enableCcToolchainResolution;
}
Expand Down Expand Up @@ -842,6 +789,7 @@ public boolean validateTopLevelHeaderInclusions() {
return cppOptions.validateTopLevelHeaderInclusions;
}

@Override
public boolean appleGenerateDsym() {
return appleGenerateDsym;
}
Expand Down
5 changes: 0 additions & 5 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1209,13 +1209,8 @@ public FragmentOptions getHost() {

host.useStartEndLib = useStartEndLib;
host.stripBinaries = StripMode.ALWAYS;
host.fdoOptimizeForBuild = fdoOptimizeForBuild;
host.fdoProfileLabel = fdoProfileLabel;
host.csFdoProfileLabel = csFdoProfileLabel;
host.xfdoProfileLabel = xfdoProfileLabel;
host.inmemoryDotdFiles = inmemoryDotdFiles;

host.enableFdoProfileAbsolutePath = enableFdoProfileAbsolutePath;
host.disableExpandIfAllAvailableInFlagSet = disableExpandIfAllAvailableInFlagSet;
host.disableLegacyCcProvider = disableLegacyCcProvider;
host.removeCpuCompilerCcToolchainAttributes = removeCpuCompilerCcToolchainAttributes;
Expand Down
4 changes: 0 additions & 4 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/FdoContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,10 +148,6 @@ public PropellerOptimizeInputFile getPropellerOptimizeInputFile() {
}

boolean hasArtifacts(CppConfiguration cppConfiguration) {
if (cppConfiguration.isToolConfigurationDoNotUseWillBeRemovedFor129045294()) {
// We don't want FDO for host configuration
return false;
}
return getBranchFdoProfile() != null
|| getPrefetchHintsArtifact() != null
|| getPropellerOptimizeInputFile() != null;
Expand Down
41 changes: 11 additions & 30 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/FdoHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,7 @@ public static FdoContext getFdoContext(
Artifact protoProfileArtifact = null;
Pair<FdoInputFile, Artifact> fdoInputs = null;
if (configuration.getCompilationMode() == CompilationMode.OPT) {
if (cppConfiguration
.getFdoPrefetchHintsLabelUnsafeSinceItCanReturnValueFromWrongConfiguration()
!= null) {
if (cppConfiguration.getFdoPrefetchHintsLabel() != null) {
FdoPrefetchHintsProvider provider = attributes.getFdoPrefetch();
prefetchHints = provider.getInputFile();
}
Expand All @@ -77,16 +75,13 @@ public static FdoContext getFdoContext(
if (ccArtifact != null || ldArtifact != null) {
propellerOptimizeInputFile = new PropellerOptimizeInputFile(ccArtifact, ldArtifact);
}
} else if (cppConfiguration
.getPropellerOptimizeLabelUnsafeSinceItCanReturnValueFromWrongConfiguration()
!= null) {
} else if (cppConfiguration.getPropellerOptimizeLabel() != null) {
PropellerOptimizeProvider provider = attributes.getPropellerOptimize();
propellerOptimizeInputFile = provider.getInputFile();
}

if (cppConfiguration.getFdoPathUnsafeSinceItCanReturnValueFromWrongConfiguration() != null) {
PathFragment fdoZip =
cppConfiguration.getFdoPathUnsafeSinceItCanReturnValueFromWrongConfiguration();
if (cppConfiguration.getFdoPath() != null) {
PathFragment fdoZip = cppConfiguration.getFdoPath();
SkyKey fdoKey = CcSkyframeFdoSupportValue.key(fdoZip);
SkyFunction.Environment skyframeEnv = ruleContext.getAnalysisEnvironment().getSkyframeEnv();
CcSkyframeFdoSupportValue ccSkyframeFdoSupportValue =
Expand All @@ -99,22 +94,16 @@ public static FdoContext getFdoContext(
Preconditions.checkState(fdoInputFile == null);
fdoInputFile =
FdoInputFile.fromAbsolutePath(ccSkyframeFdoSupportValue.getFdoZipPath().asFragment());
} else if (cppConfiguration
.getFdoOptimizeLabelUnsafeSinceItCanReturnValueFromWrongConfiguration()
!= null) {
} else if (cppConfiguration.getFdoOptimizeLabel() != null) {
FdoProfileProvider fdoProfileProvider = attributes.getFdoOptimizeProvider();
if (fdoProfileProvider != null) {
fdoInputs = getFdoInputs(ruleContext, fdoProfileProvider);
} else {
fdoInputFile = fdoInputFileFromArtifacts(ruleContext, attributes);
}
} else if (cppConfiguration
.getFdoProfileLabelUnsafeSinceItCanReturnValueFromWrongConfiguration()
!= null) {
} else if (cppConfiguration.getFdoProfileLabel() != null) {
fdoInputs = getFdoInputs(ruleContext, attributes.getFdoProfileProvider());
} else if (cppConfiguration
.getXFdoProfileLabelUnsafeSinceItCanReturnValueFromWrongConfiguration()
!= null) {
} else if (cppConfiguration.getXFdoProfileLabel() != null) {
fdoInputs = getFdoInputs(ruleContext, attributes.getXFdoProfileProvider());
}

Expand Down Expand Up @@ -173,8 +162,7 @@ public static FdoContext getFdoContext(
}
if ((branchFdoMode != BranchFdoMode.XBINARY_FDO)
&& (branchFdoMode != BranchFdoMode.AUTO_FDO)
&& cppConfiguration.getXFdoProfileLabelUnsafeSinceItCanReturnValueFromWrongConfiguration()
!= null) {
&& cppConfiguration.getXFdoProfileLabel() != null) {
ruleContext.throwWithRuleError("--xbinary_fdo only accepts *.xfdo and *.afdo");
}

Expand All @@ -185,8 +173,7 @@ public static FdoContext getFdoContext(
Artifact profileArtifact = null;
if (branchFdoMode == BranchFdoMode.LLVM_FDO) {
profileArtifact =
convertLLVMRawProfileToIndexed(
attributes, fdoInputFile, toolPaths, ruleContext, cppConfiguration, "fdo");
convertLLVMRawProfileToIndexed(attributes, fdoInputFile, toolPaths, ruleContext, "fdo");
if (ruleContext.hasErrors()) {
return null;
}
Expand All @@ -202,14 +189,13 @@ public static FdoContext getFdoContext(
"Symlinking FDO profile " + fdoInputFile.getBasename());
} else if (branchFdoMode == BranchFdoMode.LLVM_CS_FDO) {
Artifact nonCSProfileArtifact =
convertLLVMRawProfileToIndexed(
attributes, fdoInputFile, toolPaths, ruleContext, cppConfiguration, "fdo");
convertLLVMRawProfileToIndexed(attributes, fdoInputFile, toolPaths, ruleContext, "fdo");
if (ruleContext.hasErrors()) {
return null;
}
Artifact csProfileArtifact =
convertLLVMRawProfileToIndexed(
attributes, csFdoInputFile, toolPaths, ruleContext, cppConfiguration, "csfdo");
attributes, csFdoInputFile, toolPaths, ruleContext, "csfdo");
if (ruleContext.hasErrors()) {
return null;
}
Expand Down Expand Up @@ -342,12 +328,7 @@ private static Artifact convertLLVMRawProfileToIndexed(
FdoInputFile fdoProfile,
ImmutableMap<String, PathFragment> toolPaths,
RuleContext ruleContext,
CppConfiguration cppConfiguration,
String fdoUniqueArtifactName) {
if (cppConfiguration.isToolConfigurationDoNotUseWillBeRemovedFor129045294()) {
return null;
}

Artifact profileArtifact =
ruleContext.getUniqueDirectoryArtifact(
fdoUniqueArtifactName,
Expand Down

0 comments on commit 56dba00

Please sign in to comment.