Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BuildSystem] Teach swiftc command about -whole-module-optimization. … #28

Merged
merged 1 commit into from May 5, 2016

Conversation

@aciidb0mb3r
Copy link
Member

commented May 4, 2016

…When enabled swift compiler produces one object file and dependency file for the entire module instead of for each of the sources

@@ -288,13 +294,17 @@ class SwiftCompilerShellCommand : public ExternalCommand {
importPaths = std::vector<std::string>(imports.begin(), imports.end());
} else if (name == "temps-path") {
tempsPath = value;
} else if (name == "is-library") {
} else if (name == "is-library" || name == "enable-wmo") {

This comment has been minimized.

Copy link
@ddunbar

ddunbar May 4, 2016

Member

I'd prefer to use the full name, even though it is verbose. "wmo" is too opaque

Also, please update the docs when you add new keys!

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r May 4, 2016

Author Member

should this be enable-whole-module-optimization or enable-wholeModuleOptimization

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r May 4, 2016

Author Member

done

if (name == "is-library")
isLibrary = boolValue;
else if (name == "enable-wmo")
enableWholeModuleOptimization = boolValue;

This comment has been minimized.

Copy link
@ddunbar

ddunbar May 4, 2016

Member

I would rather factor out a helper function for bool reading than lump these into one else if, I find that less readable.

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r May 4, 2016

Author Member

done

@@ -348,37 +358,47 @@ class SwiftCompilerShellCommand : public ExternalCommand {
}

os << "{\n";

// Write out the entries for each source file if WMO is not enabled.

This comment has been minimized.

Copy link
@ddunbar

ddunbar May 4, 2016

Member

I think there are cases where WMO still uses the per-file entries (with parallel WMO). I unfortunately forget the details, but there are multiple modes, one where WMO runs as a single process, and one where WMO runs with one thread/process to do the codegen of the .o files (I think there is an explicit option for it).

Xcode's default these days is to use the one which does the codegen in parallel, so we should match that. In that mode, I believe the individual .o files are still used.

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r May 4, 2016

Author Member

Ah cool. Let me see if I can find that out

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r May 4, 2016

Author Member

Looks like the parallelism is decided by the -num-threads flag

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r May 4, 2016

Author Member

I'll just remove the if for now, not sure what value to pass to it.

@ddunbar

This comment has been minimized.

Copy link
Member

commented May 4, 2016

We should also have a test case for this.

You can teach the "fake" swiftc how to validate these options a tad. That doesn't test against the real thing, but it does protect against modifications to the code having unintended consequences.

@aciidb0mb3r

This comment has been minimized.

Copy link
Member Author

commented May 4, 2016

ah cool

@aciidb0mb3r aciidb0mb3r force-pushed the aciidb0mb3r:swiftc-wmo branch 2 times, most recently from 80e76b6 to ecd929b May 4, 2016

if (enableWholeModuleOptimization) {
result.push_back("-whole-module-optimization");
result.push_back("-num-threads");
result.push_back("1");

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r May 5, 2016

Author Member

@ddunbar how/what to set a value here.

This comment has been minimized.

Copy link
@ddunbar

ddunbar May 5, 2016

Member

We don't have a good answer currently, for now I think the best thing is to make it a separate option and handle both cases of parallel or non-parallel WMO. Then clients can pick if they want to hard code a threads value even if llbuild itself isn't scheduling it "properly"

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r May 5, 2016

Author Member

Ok. I dont think its going to matter if output file has entries for individual objects when non-parallel WMO is used. Though I'll double check to be sure.

This comment has been minimized.

Copy link
@ddunbar

ddunbar May 5, 2016

Member

I think it is ok to leave them there, if it is convenient, but I think we need to be careful to only link the correct .o files (for which I think there will just be one).

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r May 5, 2016

Author Member

The object file for entire module was present in both modes iirc.

This comment has been minimized.

Copy link
@ddunbar

ddunbar May 5, 2016

Member

That would surprise me, I would make sure it wasn't just left over from before.

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r May 5, 2016

Author Member

Just checked, you're right. Lots of ifs for swift-build then ._./

This comment has been minimized.

Copy link
@drewcrawford

drewcrawford May 5, 2016

atbuild would be interested in setting a value for num-threads, so that's a datapoint.

@aciidb0mb3r aciidb0mb3r force-pushed the aciidb0mb3r:swiftc-wmo branch 3 times, most recently from d20de37 to 347a28f May 5, 2016

@@ -353,6 +381,15 @@ class SwiftCompilerShellCommand : public ExternalCommand {
SmallString<16> masterDepsPath;
llvm::sys::path::append(masterDepsPath, tempsPath, "master.swiftdeps");
os << " \"\": {\n";
if (enableWholeModuleOptimization) {

This comment has been minimized.

Copy link
@ddunbar

ddunbar May 5, 2016

Member

Don't we only need this if WMO && !parallel-WMO?

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r May 5, 2016

Author Member

we need depsPath in both cases
object is not needed for parallel-WMO == true but it can just stay there


os << " \"" << source << "\": {\n";
os << " \"dependencies\": \"" << depsPath << "\",\n";
if (!enableWholeModuleOptimization) {

This comment has been minimized.

Copy link
@ddunbar

ddunbar May 5, 2016

Member

Don't we need this if !WMO || parallel-WMO?

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r May 5, 2016

Author Member

in both cases only one .d is generated for entire module

@aciidb0mb3r aciidb0mb3r force-pushed the aciidb0mb3r:swiftc-wmo branch from 347a28f to a66ad40 May 5, 2016

@aciidb0mb3r

This comment has been minimized.

Copy link
Member Author

commented May 5, 2016

Added test for both WMO and WMO parallel

@aciidb0mb3r aciidb0mb3r force-pushed the aciidb0mb3r:swiftc-wmo branch from a66ad40 to 599335b May 5, 2016

@aciidb0mb3r

This comment has been minimized.

Copy link
Member Author

commented May 5, 2016

@swift-ci please test

@aciidb0mb3r aciidb0mb3r force-pushed the aciidb0mb3r:swiftc-wmo branch from 599335b to cfd7aa4 May 5, 2016

@ddunbar

This comment has been minimized.

Copy link
Member

commented May 5, 2016

@swift-ci please test and merge

@aciidb0mb3r

This comment has been minimized.

Copy link
Member Author

commented May 5, 2016

@swift-ci please test

@jrose-apple

This comment has been minimized.

Copy link
Member

commented May 5, 2016

I think it's not worth investing in this; -whole-module-optimization plus the -num-threads option goes back to producing several .o files. cc @eeckstein

@aciidb0mb3r

This comment has been minimized.

Copy link
Member Author

commented May 5, 2016

@ddunbar CI passed but @jrose-apple says it's not worth doing?

@ddunbar

This comment has been minimized.

Copy link
Member

commented May 5, 2016

I think @jrose-apple was just referring to WMO with !num-threads, as it is "deprecated". I still think we should make it behave correctly as long as the compiler supports it.

@ddunbar ddunbar merged commit 2bdfb23 into apple:master May 5, 2016

2 checks passed

Swift Test Linux Platform Build finished.
Details
Swift Test OS X Platform Build finished.
Details

@aciidb0mb3r aciidb0mb3r deleted the aciidb0mb3r:swiftc-wmo branch May 6, 2016

drewcrawford added a commit to AnarchyTools/atbuild that referenced this pull request May 18, 2016
Fix WMO
We previously used a (pretty bad) hack for WMO.  This resulted in issues like #92.

Upstream now has proper support for WMO (see generally, apple/swift-llbuild#28, https://bugs.swift.org/browse/SR-881).

We now use the upstream feature to handle this case.  We also add -num-threads support, which upstream recently added.

Note that our implementation now only works for swift-DEVELOPMENT-SNAPSHOT-2016-05-09-a and above.

Resolve #92
@drewcrawford drewcrawford referenced this pull request May 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.