Skip to content

Commit

Permalink
Change Bazel's LIPO model to support dynamic configurations.
Browse files Browse the repository at this point in the history 
The key change is to eliminate the need to transition from the data to the target configuration by relying on out-of-band configuration state. Specifically, the old model drops LIPO options from the data configuration. In the cases when we have to switch back (i.e. TARGET_CONFIG_FOR_LIPO), those options have to get re-injected somehow. Static configurations achieve this with the global configuration transitions table. But dynamic configs have no comparable source (and they consciously eschew global state).

This cl changes the model to *keep* LIPO settings in the data config, then use a new "enableOrDisable" flag to determine whether or not to actually use them. With this model, the data -> target transition is now as simple as toggling that flag.

This change doesn't actually add dynamic config LIPO support. It's doing enough as it is, and we need to make sure it doesn't break existing LIPO semantics. Dynamic support will come as a followup.

PiperOrigin-RevId: 153504240
  • Loading branch information
@gregestren @aehlig
gregestren authored and aehlig committed Apr 19, 2017
1 parent b462128 commit ff29c0b
Show file tree
Hide file tree
Showing 5 changed files with 249 additions and 79 deletions.
27 changes: 7 additions & 20 deletions src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCppRuleClasses.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,14 @@
import static com.google.devtools.build.lib.syntax.Type.STRING;
import static com.google.devtools.build.lib.syntax.Type.STRING_LIST;

import com.google.common.base.Preconditions;
import com.google.common.base.Predicates;
import com.google.devtools.build.lib.analysis.BaseRuleClasses;
import com.google.devtools.build.lib.analysis.RuleDefinition;
import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
import com.google.devtools.build.lib.packages.Attribute.LateBoundLabel;
import com.google.devtools.build.lib.packages.AttributeMap;
import com.google.devtools.build.lib.packages.BuildType;
Expand Down Expand Up @@ -75,28 +75,15 @@ public class BazelCppRuleClasses {
new RuleClass.Configurator<BuildConfiguration, Rule>() {
@Override
public BuildConfiguration apply(Rule rule, BuildConfiguration configuration) {
if (configuration.useDynamicConfigurations()) {
// Dynamic configurations don't currently work with LIPO. partially because of lack of
// support for TARGET_CONFIG_FOR_LIPO. We can't check for LIPO here because we have
// to apply TARGET_CONFIG_FOR_LIPO to determine it, So we just assume LIPO is disabled.
// This is safe because Bazel errors out if the two options are combined.
return configuration;
}
Preconditions.checkState(!configuration.useDynamicConfigurations(),
"Dynamic configurations don't use rule class configurators for LIPO");
BuildConfiguration toplevelConfig =
configuration.getConfiguration(LipoTransition.TARGET_CONFIG_FOR_LIPO);
// If LIPO is enabled, override the default configuration.
CppConfiguration cppConfig = configuration.getFragment(CppConfiguration.class);
if (toplevelConfig != null
&& toplevelConfig.getFragment(CppConfiguration.class).isLipoOptimization()
&& !configuration.isHostConfiguration()
&& !configuration.getFragment(CppConfiguration.class).isLipoContextCollector()) {
// Switch back to data when the cc_binary is not the LIPO context.
return (rule.getLabel()
.equals(
toplevelConfig.getFragment(CppConfiguration.class).getLipoContextLabel()))
? toplevelConfig
: configuration
.getTransitions()
.getStaticConfiguration(ConfigurationTransition.DATA);
&& cppConfig.isDataConfigurationForLipoOptimization()
&& rule.getLabel().equals(cppConfig.getLipoContextForBuild())) {
return toplevelConfig;
}
return configuration;
}
Expand Down
70 changes: 54 additions & 16 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 @@ -16,6 +16,7 @@

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Predicate;
import com.google.common.base.Verify;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -445,9 +446,7 @@ protected CppConfiguration(CppConfigurationParameters params)
this.stlLabel = params.stlLabel;
this.compilationMode = params.commonOptions.compilationMode;
this.useLLVMCoverageMap = params.commonOptions.useLLVMCoverageMapFormat;
this.lipoContextCollector = cppOptions.lipoCollector;


this.lipoContextCollector = cppOptions.isLipoContextCollector();
this.crosstoolTopPathFragment = crosstoolTop.getPackageIdentifier().getPathUnderExecRoot();

try {
Expand All @@ -467,7 +466,7 @@ protected CppConfiguration(CppConfigurationParameters params)
throw new AssertionError(e);
}

if (cppOptions.lipoMode == LipoMode.BINARY) {
if (cppOptions.getLipoMode() == LipoMode.BINARY) {
// TODO(bazel-team): implement dynamic linking with LIPO
this.dynamicMode = DynamicMode.OFF;
} else {
Expand Down Expand Up @@ -1685,7 +1684,7 @@ public boolean getLinkCompileOutputSeparately() {
* AutoFDO mode.
*/
public boolean getAutoFdoLipoData() {
return cppOptions.autoFdoLipoData;
return cppOptions.getAutoFdoLipoData();
}

/**
Expand All @@ -1700,18 +1699,31 @@ public Label getStl() {
* Returns the currently active LIPO compilation mode.
*/
public LipoMode getLipoMode() {
return cppOptions.lipoMode;
return cppOptions.getLipoMode();
}

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

/**
* Returns true if LIPO optimization should be applied for this configuration.
*/
public boolean isLipoOptimization() {
// The LIPO optimization bits are set in the LIPO context collector configuration, too.
return cppOptions.isLipoOptimization();
}

/**
* Returns true if this is a data configuration for a LIPO-optimizing build.
*
* <p>This means LIPO is not applied for this configuration, but LIPO might be reenabled further
* down the dependency tree.
*/
public boolean isDataConfigurationForLipoOptimization() {
return cppOptions.isDataConfigurationForLipoOptimization();
}

public boolean isLipoOptimizationOrInstrumentation() {
return cppOptions.isLipoOptimizationOrInstrumentation();
}
Expand All @@ -1720,8 +1732,8 @@ public boolean isLipoOptimizationOrInstrumentation() {
* Returns true if it is AutoFDO LIPO build.
*/
public boolean isAutoFdoLipo() {
return cppOptions.fdoOptimize != null
&& CppFileTypes.GCC_AUTO_PROFILE.matches(cppOptions.fdoOptimize)
return cppOptions.getFdoOptimize() != null
&& CppFileTypes.GCC_AUTO_PROFILE.matches(cppOptions.getFdoOptimize())
&& getLipoMode() != LipoMode.OFF;
}

Expand Down Expand Up @@ -1755,8 +1767,29 @@ public ImmutableList<PerLabelOptions> getPerFileCopts() {
return ImmutableList.copyOf(cppOptions.perFileCopts);
}

/**
* Returns the LIPO context for this configuration.
*
* <p>This only exists for configurations that apply LIPO in LIPO-optimized builds. It does
* <b>not</b> exist for data configurations, which contain LIPO state but don't actually apply
* LIPO. Nor does it exist for host configurations, which contain no LIPO state.
*/
public Label getLipoContextLabel() {
return cppOptions.getLipoContextLabel();
return cppOptions.getLipoContext();
}

/**
* Returns the LIPO context for this build, even if LIPO isn't enabled in the current
* configuration.
*
* <p>Unlike {@link #getLipoContextLabel}, this returns the LIPO context for the data
* configuration.
*
* <p>Unless you have a clear reason to use this version (which basically involves
* inspecting oher configurations' state), always use {@link #getLipoContextLabel}.
*/
public Label getLipoContextForBuild() {
return cppOptions.getLipoContextForBuild();
}

/**
Expand Down Expand Up @@ -2038,23 +2071,23 @@ public void reportInvalidOptions(EventHandler reporter, BuildOptions buildOption
}
}

if (cppOptions.fdoInstrument != null && cppOptions.fdoOptimize != null) {
if (cppOptions.getFdoInstrument() != null && cppOptions.getFdoOptimize() != null) {
reporter.handle(Event.error("Cannot instrument and optimize for FDO at the same time. "
+ "Remove one of the '--fdo_instrument' and '--fdo_optimize' options"));
}

if (cppOptions.lipoContext != null) {
if (cppOptions.lipoMode != LipoMode.BINARY || cppOptions.fdoOptimize == null) {
if (cppOptions.lipoContextForBuild != null) {
if (cppOptions.getLipoMode() != LipoMode.BINARY || cppOptions.getFdoOptimize() == null) {
reporter.handle(Event.warn("The --lipo_context option can only be used together with "
+ "--fdo_optimize=<profile zip> and --lipo=binary. LIPO context will be ignored."));
}
} else {
if (cppOptions.lipoMode == LipoMode.BINARY && cppOptions.fdoOptimize != null) {
if (cppOptions.getLipoMode() == LipoMode.BINARY && cppOptions.getFdoOptimize() != null) {
reporter.handle(Event.error("The --lipo_context option must be specified when using "
+ "--fdo_optimize=<profile zip> and --lipo=binary"));
}
}
if (cppOptions.lipoMode == LipoMode.BINARY && compilationMode != CompilationMode.OPT) {
if (cppOptions.getLipoMode() == LipoMode.BINARY && compilationMode != CompilationMode.OPT) {
reporter.handle(Event.error(
"'--lipo=binary' can only be used with '--compilation_mode=opt' (or '-c opt')"));
}
Expand All @@ -2070,6 +2103,11 @@ public void reportInvalidOptions(EventHandler reporter, BuildOptions buildOption
+ "generate a dwp for the test executable, use '--fission=yes' with a toolchain "
+ "that supports Fission and build statically."));
}

// This is an assertion check vs. user error because users can't trigger this state.
Verify.verify(
!(buildOptions.get(BuildConfiguration.Options.class).isHost && cppOptions.isFdo()),
"FDO/LIPO state should not propagate to the host configuration");
}

@Override
Expand Down Expand Up @@ -2156,7 +2194,7 @@ public Map<String, Object> lateBoundOptionDefaults() {
}

public PathFragment getFdoInstrument() {
return cppOptions.fdoInstrument;
return cppOptions.getFdoInstrument();
}

public Path getFdoZip() {
Expand All @@ -2170,7 +2208,7 @@ public Path getFdoZip() {
@Override
public ImmutableSet<String> configurationEnabledFeatures(RuleContext ruleContext) {
ImmutableSet.Builder<String> requestedFeatures = ImmutableSet.builder();
if (cppOptions.fdoInstrument != null) {
if (cppOptions.getFdoInstrument() != null) {
requestedFeatures.add(CppRuleClasses.FDO_INSTRUMENT);
}

Expand Down
8 changes: 4 additions & 4 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,11 +153,11 @@ protected CppConfigurationParameters createParameters(
// FDO
// TODO(bazel-team): move this to CppConfiguration.prepareHook
Path fdoZip;
if (cppOptions.fdoOptimize == null) {
if (cppOptions.getFdoOptimize() == null) {
fdoZip = null;
} else if (cppOptions.fdoOptimize.startsWith("//")) {
} else if (cppOptions.getFdoOptimize().startsWith("//")) {
try {
Target target = env.getTarget(Label.parseAbsolute(cppOptions.fdoOptimize));
Target target = env.getTarget(Label.parseAbsolute(cppOptions.getFdoOptimize()));
if (target == null) {
return null;
}
Expand All @@ -175,7 +175,7 @@ protected CppConfigurationParameters createParameters(
throw new InvalidConfigurationException(e);
}
} else {
fdoZip = directories.getWorkspace().getRelative(cppOptions.fdoOptimize);
fdoZip = directories.getWorkspace().getRelative(cppOptions.getFdoOptimize());
try {
// We don't check for file existence, but at least the filename should be well-formed.
FileSystemUtils.checkBaseName(fdoZip.getBaseName());
Expand Down
Loading

0 comments on commit ff29c0b

Please sign in to comment.