[IRGen] Emit mod summary for full LTO to enable hermetic seal with lld #42366
Conversation
LTO pipeline requires consistent `EnableSplitLTOUnit` and module summary in regular full LTO bitcode, and clang enables `EnableSplitLTOUnit` and emit regular lto module summary on non-ld64 platforms. Therefore, swiftc has to emit them for the consistency with clang.
|
@swift-ci Please test |
|
Please test with following PR: @swift-ci Please test Linux platform |
|
@kubamracek Could you take a look if you have time? |
|
I'm afraid I don't understand LTO enough to review this. @compnerd? |
|
Hi @compnerd, you've also reviewed the cherry-picked swiftlang/llvm-project#4228 LLVM patch on Apple's fork. Would you be available to review this PR as well? Thank you! |
|
Ping @compnerd. |
|
Ping @gottesmm |
| // Emit a module summary by default for Regular LTO except ld64-based ones | ||
| // (which use the legacy LTO API). | ||
| bool EmitRegularLTOSummary = | ||
| targetMachine->getTargetTriple().getVendor() != llvm::Triple::Apple; |
There was a problem hiding this comment.
Why the vendor check rather than !targetMachine->getTargetTriple().isOSDarwin()?
There was a problem hiding this comment.
@kubamracek Which form do we prefer here? != Apple or isOSDarwin?
There was a problem hiding this comment.
| EmitPasses.add(createBitcodeWriterPass(out)); | ||
| if (EmitRegularLTOSummary) { | ||
| module->addModuleFlag(llvm::Module::Error, "ThinLTO", uint32_t(0)); | ||
| // Assume other sources are compiled with -fsplit-lto-unit (it's enabled |
There was a problem hiding this comment.
Can we avoid this assumption? Why do we need this assumption in the first place? Perhaps we should serialize and reference this from the Swift module?
There was a problem hiding this comment.
Other sources mean not only Swiftc-oriented LTO summary but also including Clang-oriented summary. So we can't propagate that information through swiftmodule.
Another option to remove that assumption is adding -split-lto-unit in swiftc also. What do you think?
|
@eeckstein -- Any thoughts on the proposed changes to the LTO settings here? |
|
I know too little about LTO. That would be a question for @gottesmm or @aschwaighofer |
|
Hi @gottesmm @aschwaighofer, would you have a moment to take a look at this PR? Thanks! |
|
I am not an LTO expert either. The changes seem reasonable to me but I can't predict the consequences of unconditionally (on linux) adding the module flags:
The code seems to indeed follow what clang does. Can we take this change and deal with any fallout (which I expect to be only on linux) or do you have concerns with doing so @kubamracek ? |
|
No concerns with that. |
|
Thanks for the reviews! Can we merge then? |
|
re-trigger testing because it looks like the last test is quite a bit in the past |
|
@swift-ci test |
|
Could be this merged now? |
|
@swift-ci Please test and merge |
LTO pipeline requires consistent
EnableSplitLTOUnitand module summaryin regular full LTO bitcode, and clang enables
EnableSplitLTOUnitandemit regular lto module summary on non-ld64 platforms.
Therefore, swiftc has to emit them for the consistency with clang.
This patch unlocks stdlib to be linked with a simple code with
--hermetic-seal-at-linkand lld on LinuxThere are still some remaining issues:
test/IRGen/conditional-dead-strip-exec.swiftdoesn't pass yet.test/IRGen/virtual-function-elimination-exec.swiftandtest/IRGen/witness-method-elimination-exec.swiftare failing with the following preemption error.It would be appreciated if you have any ideas.
Depends on swiftlang/llvm-project#4228