Route fdo logic that deals with the path to the fdo profile through CcToolchainProvider
As --fdo_optimize can point to a label, the path to the fdo profile can not be reliably determined in CppConfiguration.
In order to enable the fdo features (which depend on the path to the fdo profile), the logic from CppConfiguration.configurationEnabledFeatures() has been moved to CcCommon.configureFeatures(). The latter method has access to the fdo profile path through CcToolchainProvider.
RELNOTES: None.
PiperOrigin-RevId: 186278311
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index 8d7979b..2be6796 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -177,7 +177,8 @@
private final ListMultimap<String, ConfiguredFilesetEntry> filesetEntryMap;
private final ImmutableMap<Label, ConfigMatchingProvider> configConditions;
private final AspectAwareAttributeMapper attributes;
- private final ImmutableSet<String> features;
+ private final ImmutableSet<String> enabledFeatures;
+ private final ImmutableSet<String> disabledFeatures;
private final String ruleClassNameForLogging;
private final BuildConfiguration hostConfiguration;
private final PatchTransition disableLipoTransition;
@@ -217,7 +218,11 @@
this.filesetEntryMap = filesetEntryMap;
this.configConditions = configConditions;
this.attributes = new AspectAwareAttributeMapper(attributes, aspectAttributes);
- this.features = getEnabledFeatures();
+ Set<String> allEnabledFeatures = new HashSet<>();
+ Set<String> allDisabledFeatures = new HashSet<>();
+ getAllFeatures(allEnabledFeatures, allDisabledFeatures);
+ this.enabledFeatures = ImmutableSortedSet.copyOf(allEnabledFeatures);
+ this.disabledFeatures = ImmutableSortedSet.copyOf(allDisabledFeatures);
this.ruleClassNameForLogging = ruleClassNameForLogging;
this.hostConfiguration = builder.hostConfiguration;
this.disableLipoTransition = builder.disableLipoTransition;
@@ -225,7 +230,7 @@
this.toolchainContext = toolchainContext;
}
- private ImmutableSet<String> getEnabledFeatures() {
+ private void getAllFeatures(Set<String> allEnabledFeatures, Set<String> allDisabledFeatures) {
Set<String> globallyEnabled = new HashSet<>();
Set<String> globallyDisabled = new HashSet<>();
parseFeatures(getConfiguration().getDefaultFeatures(), globallyEnabled, globallyDisabled);
@@ -237,23 +242,26 @@
if (attributes().has("features", Type.STRING_LIST)) {
parseFeatures(attributes().get("features", Type.STRING_LIST), ruleEnabled, ruleDisabled);
}
+
Set<String> ruleDisabledFeatures =
Sets.union(ruleDisabled, Sets.difference(packageDisabled, ruleEnabled));
- Set<String> disabledFeatures = Sets.union(ruleDisabledFeatures, globallyDisabled);
+ allDisabledFeatures.addAll(Sets.union(ruleDisabledFeatures, globallyDisabled));
for (ImmutableMap.Entry<Class<? extends Fragment>, Fragment> entry :
getConfiguration().getAllFragments().entrySet()) {
if (isLegalFragment(entry.getKey())) {
globallyEnabled.addAll(
entry
.getValue()
- .configurationEnabledFeatures(this, ImmutableSortedSet.copyOf(disabledFeatures)));
+ .configurationEnabledFeatures(
+ this, ImmutableSortedSet.copyOf(allDisabledFeatures)));
}
}
+
Set<String> packageFeatures =
Sets.difference(Sets.union(globallyEnabled, packageEnabled), packageDisabled);
Set<String> ruleFeatures =
Sets.difference(Sets.union(packageFeatures, ruleEnabled), ruleDisabled);
- return ImmutableSortedSet.copyOf(Sets.difference(ruleFeatures, globallyDisabled));
+ allEnabledFeatures.addAll(Sets.difference(ruleFeatures, globallyDisabled));
}
private void parseFeatures(Iterable<String> features, Set<String> enabled, Set<String> disabled) {
@@ -1371,7 +1379,12 @@
* @return the set of features applicable for the current rule's package.
*/
public ImmutableSet<String> getFeatures() {
- return features;
+ return enabledFeatures;
+ }
+
+ /** @return the set of features that are disabled for the current rule's package. */
+ public ImmutableSet<String> getDisabledFeatures() {
+ return disabledFeatures;
}
@Override
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 69c3f60..a0629da 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
@@ -49,6 +49,7 @@
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables;
import com.google.devtools.build.lib.rules.cpp.CppConfiguration.DynamicMode;
import com.google.devtools.build.lib.rules.cpp.CppConfiguration.HeadersCheckingMode;
+import com.google.devtools.build.lib.rules.cpp.FdoSupport.FdoMode;
import com.google.devtools.build.lib.shell.ShellUtils;
import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
@@ -698,6 +699,7 @@
}
ImmutableSet<String> allUnsupportedFeatures = unsupportedFeaturesBuilder.build();
ImmutableSet.Builder<String> allRequestedFeaturesBuilder = ImmutableSet.builder();
+
// If STATIC_LINK_MSVCRT feature isn't specified by user, we add DYNAMIC_LINK_MSVCRT_* feature
// according to compilation mode.
// If STATIC_LINK_MSVCRT feature is specified, we add STATIC_LINK_MSVCRT_* feature
@@ -713,6 +715,38 @@
? CppRuleClasses.DYNAMIC_LINK_MSVCRT_DEBUG
: CppRuleClasses.DYNAMIC_LINK_MSVCRT_NO_DEBUG);
}
+
+ CppConfiguration cppConfiguration = toolchain.getCppConfiguration();
+ if (cppConfiguration.getFdoInstrument() != null
+ && !ruleContext.getDisabledFeatures().contains(CppRuleClasses.FDO_INSTRUMENT)) {
+ allRequestedFeaturesBuilder.add(CppRuleClasses.FDO_INSTRUMENT);
+ }
+
+ FdoMode fdoMode = toolchain.getFdoMode();
+ boolean isFdo = fdoMode != FdoMode.OFF && toolchain.getCompilationMode() == CompilationMode.OPT;
+ if (isFdo
+ && fdoMode != FdoMode.AUTO_FDO
+ && !ruleContext.getDisabledFeatures().contains(CppRuleClasses.FDO_OPTIMIZE)) {
+ allRequestedFeaturesBuilder.add(CppRuleClasses.FDO_OPTIMIZE);
+ }
+ if (isFdo && fdoMode == FdoMode.AUTO_FDO) {
+ allRequestedFeaturesBuilder.add(CppRuleClasses.AUTOFDO);
+ // For LLVM, support implicit enabling of ThinLTO for AFDO unless it has been
+ // explicitly disabled.
+ if (toolchain.isLLVMCompiler()
+ && !ruleContext.getDisabledFeatures().contains(CppRuleClasses.THIN_LTO)) {
+ allRequestedFeaturesBuilder.add(CppRuleClasses.ENABLE_AFDO_THINLTO);
+ }
+ }
+ if (cppConfiguration.isLipoOptimizationOrInstrumentation()) {
+ // Map LIPO to ThinLTO for LLVM builds.
+ if (toolchain.isLLVMCompiler() && fdoMode != FdoMode.OFF) {
+ allRequestedFeaturesBuilder.add(CppRuleClasses.THIN_LTO);
+ } else {
+ allRequestedFeaturesBuilder.add(CppRuleClasses.LIPO);
+ }
+ }
+
ImmutableList.Builder<String> allFeatures =
new ImmutableList.Builder<String>()
.addAll(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
index 885f031..22ee5ac 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
@@ -53,6 +53,7 @@
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables.Builder;
import com.google.devtools.build.lib.rules.cpp.CppConfiguration.Tool;
import com.google.devtools.build.lib.rules.cpp.FdoSupport.FdoException;
+import com.google.devtools.build.lib.rules.cpp.FdoSupport.FdoMode;
import com.google.devtools.build.lib.syntax.Type;
import com.google.devtools.build.lib.util.FileType;
import com.google.devtools.build.lib.util.FileTypeSet;
@@ -371,12 +372,20 @@
return null;
}
+ FdoMode fdoMode;
+ if (fdoZip == null) {
+ fdoMode = FdoMode.OFF;
+ } else if (CppFileTypes.GCC_AUTO_PROFILE.matches(fdoZip.getBaseName())) {
+ fdoMode = FdoMode.AUTO_FDO;
+ } else if (cppConfiguration.isLLVMOptimizedFdo(toolchainInfo.isLLVMCompiler())) {
+ fdoMode = FdoMode.LLVM_FDO;
+ } else {
+ fdoMode = FdoMode.VANILLA;
+ }
+
SkyKey fdoKey =
FdoSupportValue.key(
- cppConfiguration.getLipoMode(),
- fdoZip,
- cppConfiguration.getFdoInstrument(),
- cppConfiguration.isLLVMOptimizedFdo(toolchainInfo.isLLVMCompiler()));
+ cppConfiguration.getLipoMode(), fdoZip, cppConfiguration.getFdoInstrument(), fdoMode);
SkyFunction.Environment skyframeEnv = ruleContext.getAnalysisEnvironment().getSkyframeEnv();
FdoSupportValue fdoSupport;
@@ -562,7 +571,8 @@
: null,
getEnvironment(ruleContext),
builtInIncludeDirectories,
- sysroot);
+ sysroot,
+ fdoMode);
TemplateVariableInfo templateVariableInfo =
createMakeVariableProvider(cppConfiguration, sysroot);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java
index 2f4ca79..0f8358e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java
@@ -32,6 +32,7 @@
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables;
import com.google.devtools.build.lib.rules.cpp.CppConfiguration.Tool;
+import com.google.devtools.build.lib.rules.cpp.FdoSupport.FdoMode;
import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
import com.google.devtools.build.lib.util.Pair;
@@ -79,7 +80,8 @@
null,
ImmutableMap.<String, String>of(),
ImmutableList.<PathFragment>of(),
- null);
+ null,
+ FdoMode.OFF);
@Nullable private final CppConfiguration cppConfiguration;
private final CppToolchainInfo toolchainInfo;
@@ -109,6 +111,7 @@
private final ImmutableMap<String, String> environment;
private final ImmutableList<PathFragment> builtInIncludeDirectories;
@Nullable private final PathFragment sysroot;
+ private final FdoMode fdoMode;
public CcToolchainProvider(
ImmutableMap<String, Object> skylarkToolchain,
@@ -139,7 +142,8 @@
Artifact linkDynamicLibraryTool,
ImmutableMap<String, String> environment,
ImmutableList<PathFragment> builtInIncludeDirectories,
- @Nullable PathFragment sysroot) {
+ @Nullable PathFragment sysroot,
+ FdoMode fdoMode) {
super(skylarkToolchain, Location.BUILTIN);
this.cppConfiguration = cppConfiguration;
this.toolchainInfo = toolchainInfo;
@@ -169,6 +173,7 @@
this.environment = environment;
this.builtInIncludeDirectories = builtInIncludeDirectories;
this.sysroot = sysroot;
+ this.fdoMode = fdoMode;
}
/** Returns c++ Make variables. */
@@ -782,6 +787,10 @@
return toolchainInfo.isLLVMCompiler();
}
+ public FdoMode getFdoMode() {
+ return fdoMode;
+ }
+
// Not all of CcToolchainProvider is exposed to Skylark, which makes implementing deep equality
// impossible: if Java-only parts are considered, the behavior is surprising in Skylark, if they
// are not, the behavior is surprising in Java. Thus, object identity it is.
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 2049d04..8302cc0 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
@@ -1343,36 +1343,6 @@
public ImmutableSet<String> configurationEnabledFeatures(
RuleContext ruleContext, ImmutableSet<String> disabledFeatures) {
ImmutableSet.Builder<String> requestedFeatures = ImmutableSet.builder();
- if (cppOptions.getFdoInstrument() != null) {
- requestedFeatures.add(CppRuleClasses.FDO_INSTRUMENT);
- }
-
- String fdoZip = null;
- if (fdoProfileAbsolutePath != null) {
- fdoZip = fdoProfileAbsolutePath.getPathString();
- } else if (fdoProfileLabel != null) {
- fdoZip = fdoProfileLabel.getName();
- }
- boolean isFdo = fdoZip != null && compilationMode == CompilationMode.OPT;
- if (isFdo && !CppFileTypes.GCC_AUTO_PROFILE.matches(fdoZip)) {
- requestedFeatures.add(CppRuleClasses.FDO_OPTIMIZE);
- }
- if (isFdo && CppFileTypes.GCC_AUTO_PROFILE.matches(fdoZip)) {
- requestedFeatures.add(CppRuleClasses.AUTOFDO);
- // For LLVM, support implicit enabling of ThinLTO for AFDO unless it has been
- // explicitly disabled.
- if (isLLVMCompiler() && !disabledFeatures.contains(CppRuleClasses.THIN_LTO)) {
- requestedFeatures.add(CppRuleClasses.ENABLE_AFDO_THINLTO);
- }
- }
- if (isLipoOptimizationOrInstrumentation()) {
- // Map LIPO to ThinLTO for LLVM builds.
- if (isLLVMCompiler() && cppOptions.getFdoOptimize() != null) {
- requestedFeatures.add(CppRuleClasses.THIN_LTO);
- } else {
- requestedFeatures.add(CppRuleClasses.LIPO);
- }
- }
if (ruleContext.getConfiguration().isCodeCoverageEnabled()) {
requestedFeatures.add(CppRuleClasses.COVERAGE);
if (useLLVMCoverageMap) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java
index 41b1605..bbaa0bf 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java
@@ -283,20 +283,10 @@
PathFragment fdoInstrument,
Path fdoProfile,
LipoMode lipoMode,
- boolean llvmFdo,
Path execRoot,
- String productName)
+ String productName,
+ FdoMode fdoMode)
throws IOException, FdoException, InterruptedException {
- FdoMode fdoMode;
- if (fdoProfile != null && isAutoFdo(fdoProfile.getBaseName())) {
- fdoMode = FdoMode.AUTO_FDO;
- } else if (fdoProfile != null && llvmFdo) {
- fdoMode = FdoMode.LLVM_FDO;
- } else if (fdoProfile != null) {
- fdoMode = FdoMode.VANILLA;
- } else {
- fdoMode = FdoMode.OFF;
- }
if (fdoProfile == null) {
lipoMode = LipoMode.OFF;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportFunction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportFunction.java
index 63c2d3f33..f864104 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportFunction.java
@@ -65,9 +65,9 @@
key.getFdoInstrument(),
key.getFdoZip(),
key.getLipoMode(),
- key.getLLVMFdo(),
execRoot,
- directories.getProductName());
+ directories.getProductName(),
+ key.getFdoMode());
if (env.valuesMissing()) {
return null;
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportValue.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportValue.java
index 0f94dcd..6cd722f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportValue.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportValue.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.rules.cpp;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
+import com.google.devtools.build.lib.rules.cpp.FdoSupport.FdoMode;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.LipoMode;
@@ -44,13 +45,13 @@
private final LipoMode lipoMode;
private final Path fdoZip;
private final PathFragment fdoInstrument;
- private final boolean llvmFdo;
+ private final FdoMode fdoMode;
- private Key(LipoMode lipoMode, Path fdoZip, PathFragment fdoInstrument, boolean llvmFdo) {
+ private Key(LipoMode lipoMode, Path fdoZip, PathFragment fdoInstrument, FdoMode fdoMode) {
this.lipoMode = lipoMode;
this.fdoZip = fdoZip;
this.fdoInstrument = fdoInstrument;
- this.llvmFdo = llvmFdo;
+ this.fdoMode = fdoMode;
}
public LipoMode getLipoMode() {
@@ -65,8 +66,8 @@
return fdoInstrument;
}
- public boolean getLLVMFdo() {
- return llvmFdo;
+ public FdoMode getFdoMode() {
+ return fdoMode;
}
@Override
@@ -82,7 +83,7 @@
Key that = (Key) o;
return Objects.equals(this.lipoMode, that.lipoMode)
&& Objects.equals(this.fdoZip, that.fdoZip)
- && Objects.equals(this.llvmFdo, that.llvmFdo)
+ && Objects.equals(this.fdoMode, that.fdoMode)
&& Objects.equals(this.fdoInstrument, that.fdoInstrument);
}
@@ -103,7 +104,7 @@
}
public static SkyKey key(
- LipoMode lipoMode, Path fdoZip, PathFragment fdoInstrument, boolean llvmFdo) {
- return LegacySkyKey.create(SKYFUNCTION, new Key(lipoMode, fdoZip, fdoInstrument, llvmFdo));
+ LipoMode lipoMode, Path fdoZip, PathFragment fdoInstrument, FdoMode fdoMode) {
+ return LegacySkyKey.create(SKYFUNCTION, new Key(lipoMode, fdoZip, fdoInstrument, fdoMode));
}
}
