[LTO] Support LLVM LTO for driver #32430

kateinoigakukun · 2020-06-17T13:49:29Z

This commit adds LTO support for handling linker options and LLVM BC
emission. Even for ELF, swift-autolink-extract is unnecessary because
linker options are embedded in LLVM BC content when LTO.

This is pulled out from #32237 and based on #32429

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

gottesmm · 2020-07-09T19:58:24Z

I am not a driver person so I am asking around for a driver person to look at this.

gottesmm · 2020-07-09T20:35:57Z

lib/Driver/UnixToolChains.cpp

-  if (const Arg *A = context.Args.getLastArg(options::OPT_use_ld)) {
+  if (context.OI.LTOVariant != OutputInfo::LTOKind::None) {
+    // Force to use lld for LTO
+    Linker = "lld";


Why should we force the use of lld here? Will this run on Darwin? On Darwin we definitely want to use ld64.

This will run on Unix-like platform (Linux, Android or etc not including Darwin). And on these platforms, we don't support gold LTO or something else LTO except for lld. So I forced the use of lld here.

For example, gold LTO needs libLLVMGold.so but we don't have it in swift toolchain and it's licensed under GPL, so it's a little complex problem.

At this time, I only support lld on Unix-like platforms and those other linkers may be supported in the future.

gottesmm · 2020-07-09T20:36:50Z

test/Driver/link-time-opt.swift

+// RUN: echo "int dummy;" >%t/a.cpp
+// RUN: cc -c %t/a.cpp -o %t/a.o
+// RUN: %swiftc_driver -save-temps -driver-print-jobs %S/../Inputs/empty.swift %t/a.o -lto=llvm-full -target x86_64-apple-macosx10.9 -driver-filelist-threshold=0 -o filelist 2>&1 | tee %t/forFilelistCapture | %FileCheck -check-prefix FILELIST %s
+// RUN: tail -2 %t/forFilelistCapture | head -1 | sed 's/.*-output-filelist //' | sed 's/ .*//' > %t/output-filelist


I want an end to end test. Maybe in the interpreter dir. With something that LTO can hit for sure.

Added dead symbol strip e2e test case

beccadax

I don't know much about how LTO linking works, but I can comment on driver structure.

In addition to the inline comments, please take a look at validateEmbedBitcode() and think about whether file_types::TY_LLVM_BC ought to now be valid there.

beccadax · 2020-07-09T20:39:31Z

lib/Driver/DarwinToolChains.cpp

+    llvm::sys::path::remove_filename(LTOLibPath); // 'clang'
+    llvm::sys::path::remove_filename(LTOLibPath); // 'bin'
+    llvm::sys::path::append(LTOLibPath, "lib", "libLTO.dylib");
+


Would deriving this path from ToolChain::getClangLibraryPath() give you a correct path to this dylib? (Basically, will it be present in open-source toolchains, and if so, will the toolchain version be the one we want to use?)

If not, could you generalize findARCLiteLibPath() (e.g. findXcodeClangLibPath("arc", ...), findXcodeClangLibPath("libLTO.dylib", ...)) and then use the general form here? That would reduce duplication.

beccadax · 2020-07-09T20:50:54Z

lib/Driver/Driver.cpp

@@ -1785,6 +1787,19 @@ void Driver::buildOutputInfo(const ToolChain &TC, const DerivedArgList &Args,

  }

+  if (const Arg *A = Args.getLastArg(options::OPT_lto)) {


Rather than reading OPT_lto twice, why don't we move this up top and integrate it with setting LinkerInputType and/or OI.CompilerOutputType? That will ensure consistency even if we diagnose an error.

beccadax · 2020-07-09T20:53:16Z

lib/Driver/Driver.cpp

+        llvm::StringSwitch<Optional<OutputInfo::LTOKind>>(A->getValue())
+            .Case("llvm-thin", OutputInfo::LTOKind::LLVMThin)
+            .Case("llvm-full", OutputInfo::LTOKind::LLVMFull)
+            .Default(llvm::None);


How does LTOKind::None get set? Is it just the default? Should it be possible to set it explicitly?

LTOKind::None is the default kind and it means that driver didn't get -lto= option.
In this switch statement, llvm::None means that driver got invalid lto mode string and it's different from LTOKind::None.

beccadax · 2020-07-09T20:53:41Z

lib/Driver/Driver.cpp

@@ -2119,15 +2134,16 @@ void Driver::buildActions(SmallVectorImpl<const Action *> &TopLevelActions,
    MergeModuleAction = C.createAction<MergeModuleJobAction>(AllModuleInputs);
  }

+  auto shouldPerformLTO = Args.hasArg(options::OPT_lto);


Can we read this information out of OI.LTOVariant instead of looking for OPT_lto again?

beccadax · 2020-07-09T21:00:04Z

lib/Driver/ToolChains.cpp

@@ -522,6 +522,11 @@ ToolChain::constructInvocation(const CompileJobAction &job,
    Arguments.push_back("-track-system-dependencies");
  }

+  if (auto arg = context.Args.getLastArg(options::OPT_lto)) {


Can we read this information out of context.OI.LTOVariant instead of looking for OPT_lto again?

Also, would it make sense to do this in addCommonFrontendArgs() instead? (If other calls to the frontend, like merge-modules and PCH building, ought to know if we're building for LTO, then it probably should be.)

beccadax · 2020-07-09T21:02:56Z

lib/Driver/UnixToolChains.cpp

@@ -368,15 +388,18 @@ toolchains::GenericUnix::constructInvocation(const StaticLinkJobAction &job,
  ArgStringList Arguments;

  // Configure the toolchain.
-  const char *AR = "ar";
+  const char *AR = "llvm-ar";


Is this change correct for all builds, or only for LTO?

beccadax · 2020-07-09T21:09:58Z

lib/Driver/UnixToolChains.cpp

-  if (const Arg *A = context.Args.getLastArg(options::OPT_use_ld)) {
+  if (context.OI.LTOVariant != OutputInfo::LTOKind::None) {
+    // Force to use lld for LTO
+    Linker = "lld";
+  } else if (const Arg *A = context.Args.getLastArg(options::OPT_use_ld)) {


Either OPT_use_ld should take precedence over this LTO-specific branch, or the combination of OPT_use_ld and OPT_lto should diagnose an error. Silently ignoring -use-ld is a bad move.

OK, I changed to prefer -use-ld option.

beccadax · 2020-07-09T21:14:51Z

lib/Driver/WindowsToolChains.cpp

+    if (Linker.empty())
+      Arguments.push_back("-fuse-ld=lld");


Perhaps you should move this condition out of the switch and make it if (Linker.empty() && context.OI.LTOVariant != OutputInfo::LTOKind::None). Then you could get rid of the other if statement below and simply make the appropriate Arguments.push_back() call in each case, making the control flow much more straightforward.

For that matter, maybe you should move this all the way up to line 79 or so, where you can set Linker right before a -fuse-ld= would normally be generated, rather than adding another place it could be inserted down here and having to reason about whether or not you already did it earlier. Again, please either make OPT_use_ld "win" over this logic, or diagnose an error if you use both.

kateinoigakukun · 2020-07-12T06:34:06Z

@brentdax

In addition to the inline comments, please take a look at validateEmbedBitcode() and think about whether file_types::TY_LLVM_BC ought to now be valid there.

When LTO, frontend always emit bitcode, so ignoring bitcode embedding is expected behavior. Thanks for letting me know that case.

kateinoigakukun · 2020-07-12T07:30:49Z

Could you trigger CI for all platforms?

MaxDesiatov · 2020-07-12T20:26:12Z

@swift-ci please test

MaxDesiatov · 2020-07-13T07:36:42Z

@swift-ci please test Windows platform

kateinoigakukun · 2020-07-13T10:43:41Z

Hmm, the window CI failed due to a broken zipfile. I think it's worth triggering again.

ci@SWIFTCI-WINDOWS S:\jenkins\workspace\swift-PR-windows>"C:\Program Files\Git\usr\bin\unzip.exe" -o icu4c-64_2-Win64-MSVC2017.zip -d "S:\jenkins\workspace\swift-PR-windows\icu-64_2" 
Archive:  icu4c-64_2-Win64-MSVC2017.zip
  End-of-central-directory signature not found.  Either this file is not
  a zipfile, or it constitutes one disk of a multi-part archive.  In the
  latter case the central directory and zipfile comment will be found on
  the last disk(s) of this archive.
unzip:  cannot find zipfile directory in one of icu4c-64_2-Win64-MSVC2017.zip or
        icu4c-64_2-Win64-MSVC2017.zip.zip, and cannot find icu4c-64_2-Win64-MSVC2017.zip.ZIP, period.

MaxDesiatov · 2020-07-13T10:44:55Z

@swift-ci please test Windows platform

MaxDesiatov · 2020-07-14T08:20:57Z

@swift-ci please test Windows platform

MaxDesiatov · 2020-07-14T08:32:17Z

@swift-ci please test

swift-ci · 2020-07-14T08:35:07Z

Build failed
Swift Test Linux Platform
Git Sha - 936f379ff7c260624b23f5ef5367313b91eea5e5

swift-ci · 2020-07-14T08:35:21Z

Build failed
Swift Test OS X Platform
Git Sha - 936f379ff7c260624b23f5ef5367313b91eea5e5

beccadax

A couple of comments, but they might not end up requiring changes.

beccadax · 2020-07-24T21:12:01Z

lib/Driver/WindowsToolChains.cpp

@@ -76,6 +76,20 @@ toolchains::Windows::constructInvocation(const DynamicLinkJobAction &job,
  if (const Arg *A = context.Args.getLastArg(options::OPT_use_ld)) {
    Linker = A->getValue();
  }
+
+  if (Linker.empty() && context.OI.LTOVariant != OutputInfo::LTOKind::None)
+    Arguments.push_back("-fuse-ld=lld");


Why don't we just set Linker here and then let the code after the switch add the flag?

beccadax · 2020-07-24T21:15:44Z

lib/Driver/Driver.cpp

@@ -2152,7 +2170,8 @@ void Driver::buildActions(SmallVectorImpl<const Action *> &TopLevelActions,
        (Triple.getObjectFormat() == llvm::Triple::ELF && !Triple.isPS4()) ||
        Triple.getObjectFormat() == llvm::Triple::Wasm ||
        Triple.isOSCygMing();
-    if (!AutolinkExtractInputs.empty() && AutolinkExtractRequired) {
+    if (!AutolinkExtractInputs.empty() && AutolinkExtractRequired &&
+        !shouldPerformLTO) {


Does shouldPerformLTO influence the value of RequiresAutolinkExtract? Should it?

MaxDesiatov · 2020-07-24T21:20:28Z

Would reviews from anyone else be needed before this can be merged? (Assuming that pending comments are resolved)

kateinoigakukun · 2020-07-25T11:33:03Z

@brentdax Thanks for review again. I addressed commented points.

And thank you @MaxDesiatov for facilitating review.

MaxDesiatov · 2020-07-25T13:58:53Z

@swift-ci please test

swift-ci · 2020-07-25T14:01:43Z

Build failed
Swift Test OS X Platform
Git Sha - af3ee5084d7fa088f759b386a4ab535368a32a76

swift-ci · 2020-07-25T14:01:45Z

Build failed
Swift Test Linux Platform
Git Sha - af3ee5084d7fa088f759b386a4ab535368a32a76

MaxDesiatov · 2020-07-25T14:03:16Z

@swift-ci please test Windows platform

compnerd · 2020-07-15T14:48:56Z

lib/Driver/Driver.cpp

+                     A->getAsString(Args), A->getValue());
+  }
+
+  auto LinkerInputType = OI.LTOVariant != OutputInfo::LTOKind::None


What do you think of changing this to CompilerOutputType? The linker input type is confusing since the linker should be provided both object files and bitcode.

I agree. Updated.

This commit adds LTO support for handling linker options and LLVM BC emission. Even for ELF, swift-autolink-extract is unnecessary because linker options are embeded in LLVM BC content when LTO.

MaxDesiatov · 2020-07-31T11:05:08Z

@swift-ci please test

MaxDesiatov · 2020-07-31T11:19:37Z

@swift-ci please test Windows platform

MaxDesiatov · 2020-08-04T14:18:14Z

@compnerd thanks for the review. Would you mind having another look?

kateinoigakukun · 2020-08-11T03:26:33Z

@brentdax @compnerd @gottesmm Would you mind taking a look again?

MaxDesiatov · 2020-09-14T18:16:09Z

@compnerd @gottesmm would you have a moment to have a look at this PR? Thank you!

gottesmm

LGTM

kateinoigakukun marked this pull request as draft June 17, 2020 13:49

kateinoigakukun force-pushed the katei/llvm-lto-driver branch from 332eacb to 6fca6d9 Compare July 7, 2020 23:31

kateinoigakukun marked this pull request as ready for review July 7, 2020 23:31

kateinoigakukun force-pushed the katei/llvm-lto-driver branch from 6fca6d9 to 9a2c341 Compare July 8, 2020 00:55

kateinoigakukun marked this pull request as draft July 8, 2020 01:51

kateinoigakukun force-pushed the katei/llvm-lto-driver branch 2 times, most recently from 217bd5d to a27e316 Compare July 8, 2020 04:59

kateinoigakukun marked this pull request as ready for review July 8, 2020 05:10

kateinoigakukun force-pushed the katei/llvm-lto-driver branch 2 times, most recently from 092dfa9 to fb185b1 Compare July 8, 2020 07:21

gottesmm reviewed Jul 9, 2020

View reviewed changes

beccadax requested changes Jul 9, 2020

View reviewed changes

kateinoigakukun force-pushed the katei/llvm-lto-driver branch 2 times, most recently from eddd307 to f6ee152 Compare July 12, 2020 06:27

kateinoigakukun force-pushed the katei/llvm-lto-driver branch from f6ee152 to 936f379 Compare July 12, 2020 07:27

kateinoigakukun force-pushed the katei/llvm-lto-driver branch 2 times, most recently from dfa46b6 to af3ee50 Compare July 14, 2020 00:30

swiftlang deleted a comment from swift-ci Jul 14, 2020

MaxDesiatov requested review from compnerd and beccadax July 14, 2020 11:16

beccadax approved these changes Jul 24, 2020

View reviewed changes

MaxDesiatov requested a review from gottesmm July 24, 2020 21:18

kateinoigakukun force-pushed the katei/llvm-lto-driver branch from af3ee50 to c16c007 Compare July 25, 2020 11:29

compnerd reviewed Jul 28, 2020

View reviewed changes

[LTO] Support LLVM LTO for driver

d6cddaa

This commit adds LTO support for handling linker options and LLVM BC emission. Even for ELF, swift-autolink-extract is unnecessary because linker options are embeded in LLVM BC content when LTO.

kateinoigakukun force-pushed the katei/llvm-lto-driver branch from c16c007 to d6cddaa Compare July 31, 2020 01:18

MaxDesiatov requested a review from compnerd July 31, 2020 11:06

gottesmm approved these changes Sep 14, 2020

View reviewed changes

gottesmm merged commit f196c16 into swiftlang:master Sep 18, 2020

kateinoigakukun mentioned this pull request Sep 23, 2020

Handle LLVM LTO option to perform thin and full link-time-optimization swiftlang/swift-driver#269

Merged

finagolfin mentioned this pull request Jul 25, 2023

[android][test] Fix a handful of tests and disable one CxxToSwiftToCxx bridging test #65968

Merged

		@@ -1785,6 +1787,19 @@ void Driver::buildOutputInfo(const ToolChain &TC, const DerivedArgList &Args,

		}

		if (const Arg *A = Args.getLastArg(options::OPT_lto)) {

[LTO] Support LLVM LTO for driver #32430

[LTO] Support LLVM LTO for driver #32430

Uh oh!

Conversation

kateinoigakukun commented Jun 17, 2020

Uh oh!

gottesmm commented Jul 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kateinoigakukun Jul 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beccadax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kateinoigakukun commented Jul 12, 2020

Uh oh!

kateinoigakukun commented Jul 12, 2020

Uh oh!

MaxDesiatov commented Jul 12, 2020

Uh oh!

MaxDesiatov commented Jul 13, 2020

Uh oh!

kateinoigakukun commented Jul 13, 2020

Uh oh!

MaxDesiatov commented Jul 13, 2020

Uh oh!

MaxDesiatov commented Jul 14, 2020

Uh oh!

MaxDesiatov commented Jul 14, 2020

Uh oh!

swift-ci commented Jul 14, 2020

Uh oh!

swift-ci commented Jul 14, 2020

Uh oh!

beccadax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxDesiatov commented Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kateinoigakukun commented Jul 25, 2020

Uh oh!

MaxDesiatov commented Jul 25, 2020

Uh oh!

swift-ci commented Jul 25, 2020

Uh oh!

swift-ci commented Jul 25, 2020

Uh oh!

MaxDesiatov commented Jul 25, 2020

kateinoigakukun Jul 12, 2020 •

edited

Loading

MaxDesiatov commented Jul 24, 2020 •

edited

Loading