[LTO] Support LLVM LTO for IRGen and frontend #32429

kateinoigakukun · 2020-06-17T13:33:28Z

This commit adds -lto flag for frontend to enable LTO at LLVM level.
When -lto=llvm given, compiler emits LLVM bitcode file instead of object
file and adds index summary for LTO.
In addition for ELF format, emit llvm.dependent-libraries section to
embed auto linking information

This is pulled out from #32237

CC: @compnerd @gottesmm

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

Resolves SR-NNNN.

gottesmm

I need some changes here. I want to go back through this again after you have responded to my comments. The big issue I have with this patch overall is that the special case handling code is /all/ over the patch. Generally, one wants the special case handling conditional code to be in one place so it is easy to find. My comments on this PR are basically around making the code easier to read/understand.

test/IRGen/lto_autolink.swift

gottesmm · 2020-06-18T03:44:13Z

lib/IRGen/IRGenModule.cpp

-      (TargetInfo.OutputObjectFormat == llvm::Triple::ELF && !Triple.isPS4()) ||
-      TargetInfo.OutputObjectFormat == llvm::Triple::Wasm ||
-      Triple.isOSCygMing();
+      ((TargetInfo.OutputObjectFormat == llvm::Triple::ELF &&


Can you extract this into a helper function and eliminate the variable. My suggestion:

bool doesTargetAutolinkUsingAutolinkExtract(...)

I would also use early returns to make it easier to read the code. E.x.:

if (Triple.isOSCygMing()) { return ... } ...

gottesmm · 2020-06-18T03:46:59Z

lib/IRGen/IRGenModule.cpp

@@ -1188,7 +1198,15 @@ static bool isFirstObjectFileInModule(IRGenModule &IGM) {
 void IRGenModule::emitAutolinkInfo() {
  // Collect the linker options already in the module (from ClangCodeGen).
  // FIXME: This constant should be vended by LLVM somewhere.
-  auto *Metadata = Module.getOrInsertNamedMetadata("llvm.linker.options");
+  // When performing LTO, we always use lld that supports auto linking mechanism


If we are going to do this, there needs to be an assert somewhere that this is true.

The frontend can't know which linker is used, so I'll assert it in driver

gottesmm · 2020-06-18T03:47:35Z

lib/IRGen/IRGenModule.cpp

+  // When performing LTO, we always use lld that supports auto linking mechanism
+  // with ELF. So embed dependent libraries names in "llvm.dependent-libraries"
+  // instead of "llvm.linker.options".
+  const StringRef AutolinkSectionName =


I would extract this out into a helper. Also const on a value type doesn't do anything. StringRef doesn't allow for the data it points to to be modified. In fact, we could even create a helper called getAutolinkSectionNameMetadata(...).

In fact, looking at this it seems that these are two ways of specifying at the llvm-ir auto link like things with dependent-libraries being slightly more constrained. Why not represent that explicitly via an enum. My thought is you would have 3 enum values: llvm.dependent-libraries, llvm linker options native, llvm linker options swift auto link extract.

lib/IRGen/IRGenModule.cpp

gottesmm · 2020-06-18T04:00:43Z

lib/IRGen/IRGenModule.cpp

+      // mechanism with ELF. So embed dependent libraries names in
+      // "llvm.dependent-libraries" instead of options to avoid using
+      // swift-autolink-extract.
+      AutolinkEntries.push_back(


I really dislike that this is being done here in this way. It leaks an implementation detail. Why not change AutolinkEntries to instead of taking an MDNode, take a type called:

struct AutolinkEntry { PointerIntPair<llvm::MDNode *, 1> mdNodeOrLibName; }

Then you can use the lib name as a discriminator and move this code to emitAutolinkInfo maybe in a helper function. Your thoughts?

gottesmm · 2020-06-18T07:01:43Z

lib/IRGen/IRGenModule.cpp

+}
+
+namespace {
+enum class AutolinkKind {


The nice way of doing this would be to use a struct enum pattern. The way you do this is that you:

struct AutolinkKind { enum ValueTy { ... }; ValueTy value; AutolinkKind(ValueTy value) : Value(value) {} AutolinkKind(const AutolinkKind &kind) : Value(kind.value) {} }

Then you can sink a lot of this logic into method names on Autolink. In fact you could even have private constructors and make determining the Kind a class method that selects the relevant value.

lib/IRGen/IRGenModule.cpp

kateinoigakukun · 2020-06-19T02:52:21Z

@gottesmm I addressed commented points. Could you review again?

gottesmm · 2020-06-20T04:14:40Z

lib/IRGen/IRGenModule.cpp

+  llvm::LLVMContext &ctx = Module.getContext();
+
+  // Construct autolinking entries
+  switch (Autolink) {


Can you sink this into the AutolinkKind type. I shouldn't see any switches inline here.

gottesmm

This is starting to look really nice! I really like how you separated the lower level logic onto the enum! It makes it so much clearer about what the code is actually trying to do!

I put in a bunch of comments where there are breakage of LLVM style around control flow. Can you please fix those?

lib/IRGen/IRGenModule.cpp

gottesmm · 2020-06-21T02:08:19Z

lib/IRGen/IRGenModule.cpp

+    switch (Value) {
+    case AutolinkKind::LLVMDependentLibraries:
+      return "llvm.dependent-libraries";
+      break;


You don't need a break here.

I agree that these constants should be in LLVM (probably constexpr StringLiterals).

gottesmm · 2020-06-21T02:08:29Z

lib/IRGen/IRGenModule.cpp

+    case AutolinkKind::LLVMLinkerOptions:
+    case AutolinkKind::SwiftAutoLinkExtract:
+      return "llvm.linker.options";
+      break;


Also you do not need this break.

lib/IRGen/IRGenModule.cpp

gottesmm · 2020-06-21T03:03:16Z

lib/IRGen/IRGenModule.cpp

+      case LibraryKind::Framework: {
+        llvm_unreachable(
+            "llvm.dependent-libraries doesn't support framework dependency");
+        return;


I don't think you need a return here.

gottesmm · 2020-06-21T03:05:26Z

lib/IRGen/IRGenModule.cpp

+}
+
+void AutolinkKind::writeEntries(
+    const llvm::SmallVector<llvm::MDNode *, 4> &Entries,


If you aren't going to actually write to this, I would change it to be an ArrayRef<llvm::MDNode *>. This is more general and doesn't have the small vector size in the type.

If one needed to append to the array, the way people do it is that they use:

SmallVectorImpl<llvm::MDNode *> &Entries

that way we avoid encoding the 4. That being said I think this is an immutable use here, so this is just an FYI.

gottesmm · 2020-06-21T03:07:57Z

lib/IRGen/IRGenModule.cpp

+}
+
+void AutolinkKind::collectEntriesFromLibraries(
+    llvm::SmallVector<llvm::MDNode *, 4> &Entries,


This is where I think one would use a SmallVectorImpl. Also, llvm:: is embedded into the swift namespace so you don't need llvm::.

gottesmm · 2020-06-21T03:08:12Z

lib/IRGen/IRGenModule.cpp

+
+void AutolinkKind::collectEntriesFromLibraries(
+    llvm::SmallVector<llvm::MDNode *, 4> &Entries,
+    const SmallVector<LinkLibrary, 32> &AutolinkEntries, IRGenModule &IGM) {


AutolinkEntries on the other hand, should be an ArrayRef.

gottesmm · 2020-06-21T03:10:04Z

include/swift/AST/IRGenOptions.h

@@ -65,6 +65,8 @@ enum class IRGenDebugInfoFormat : unsigned {
  CodeView
 };

+enum class IRGenLLVMLTOKind : unsigned { None, Thin, Full };


Too bad clang-format does this. I may try to fix this at some point. Don't worry about it though. Not on you.

FYI, adding trailing comma after Full results:

enum class IRGenLLVMLTOKind : unsigned { None, Thin, Full, };

gottesmm · 2020-06-21T03:19:00Z

lib/IRGen/IRGenModule.cpp

+
+void AutolinkKind::collectEntriesFromLibraries(
+    SmallVectorImpl<llvm::MDNode *> &Entries,
+    const ArrayRef<LinkLibrary> &AutolinkEntries, IRGenModule &IGM) {


ArrayRef is a value type that refers to an unowned const array. You don't need const or to pass by reference.

gottesmm · 2020-06-22T21:03:54Z

lib/IRGen/IRGenModule.cpp

+  StringRef AutolinkSectionName = Autolink.getSectionNameMetadata();
+
+  auto *Metadata = Module.getOrInsertNamedMetadata(AutolinkSectionName);
+  llvm::SetVector<llvm::MDNode *> Entries;


I think this was a SmallVector before, no? Why not make a SmallSetVector? Or maybe my memory is wrong.

Ah, yes. I restored to use SmallSetVector

kateinoigakukun · 2020-06-24T03:04:50Z

I added some cases around clang importer things

kateinoigakukun · 2020-06-29T23:14:37Z

@gottesmm sorry for pinging you. Could you take a look again?

gottesmm · 2020-06-30T18:55:59Z

lib/IRGen/IRGenModule.cpp

+  static AutolinkKind create(const SwiftTargetInfo &TargetInfo,
+                             llvm::Triple Triple, IRGenLLVMLTOKind LLVMLTOKind);
+};
+} // namespace


Can you put a newline in between }; and the }. Can you change // namespace -> // anonymous namespace.?

Updated 👍

gottesmm

Make that small change and then LGTM!

compnerd · 2020-07-01T16:51:49Z

@swift-ci please test

swift-ci · 2020-07-01T16:54:42Z

Build failed
Swift Test Linux Platform
Git Sha - 41f730b648bc95d81575978d7e0c5ef5d6c987cb

swift-ci · 2020-07-01T16:55:10Z

Build failed
Swift Test OS X Platform
Git Sha - 41f730b648bc95d81575978d7e0c5ef5d6c987cb

This commit adds -lto flag for frontend to enable LTO at LLVM level. When -lto=llvm given, compiler emits LLVM bitcode file instead of object file and adds index summary for LTO. In addition for ELF format, emit llvm.dependent-libraries section to embed auto linking information

kateinoigakukun · 2020-07-01T23:31:23Z

It seems the failing test case stdlib/KeyPath.swift is not related to this change. ref: #32664

compnerd · 2020-07-02T20:23:43Z

@swift-ci please test

swift-ci · 2020-07-02T20:26:45Z

Build failed
Swift Test Linux Platform
Git Sha - c1ca57ff45d18459ce837777416c56f97074e7b3

swift-ci · 2020-07-02T20:27:09Z

Build failed
Swift Test OS X Platform
Git Sha - c1ca57ff45d18459ce837777416c56f97074e7b3

kateinoigakukun · 2020-07-06T13:55:41Z

@compnerd @gottesmm I think this PR is ready to merge. Could you merge this?

compnerd · 2020-07-07T22:23:11Z

The tests didnt trigger on the windows builder - this seems to be causing a windows test failure:

https://ci-external.swift.org/job/oss-swift-windows-x86_64-vs2019/1968/
https://ci-external.swift.org/job/oss-swift-windows-x86_64-vs2019/1968/consoleText

The test was overly strict in checking the IR, loosen the test appropriately.

kateinoigakukun mentioned this pull request Jun 17, 2020

[LTO] Support LLVM LTO for driver #32430

Merged

gottesmm reviewed Jun 18, 2020

View reviewed changes

kateinoigakukun force-pushed the katei/llvm-lto-driver-part branch from e23c5a9 to c1ea01b Compare June 18, 2020 06:51

gottesmm reviewed Jun 18, 2020

View reviewed changes

lib/IRGen/IRGenModule.cpp Show resolved Hide resolved

kateinoigakukun force-pushed the katei/llvm-lto-driver-part branch 3 times, most recently from f806bc4 to 41f730b Compare June 18, 2020 07:53

gottesmm reviewed Jun 20, 2020

View reviewed changes

kateinoigakukun force-pushed the katei/llvm-lto-driver-part branch 2 times, most recently from f30bfb0 to 2207677 Compare June 20, 2020 06:14