Skip to content

[Frontend] Set up output file .swiftmodule.summary #33324

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 7, 2020
Merged

[Frontend] Set up output file .swiftmodule.summary #33324

merged 1 commit into from
Sep 7, 2020

Conversation

kateinoigakukun
Copy link
Member

@kateinoigakukun kateinoigakukun commented Aug 6, 2020 •
edited
Loading

Module summary file contains symbol dependency information and it's similar to LLVM thin LTO information.

This new file will be used for cross dead function elimination

This patch focus on teaching frontend and driver to emit this file. The actual serialization and deserialization implementation will come later PRs.

For more info: https://forums.swift.org/t/refactoring-plan-of-silvisitor-for-lto/37678/14

CC: @compnerd @gottesmm

@kateinoigakukun kateinoigakukun force-pushed the katei/swift-module-summary/frontend branch from d917510 to 165af08 Compare August 6, 2020 10:17
@MaxDesiatov
Copy link
Contributor

@swift-ci please test

@kateinoigakukun kateinoigakukun force-pushed the katei/swift-module-summary/frontend branch 2 times, most recently from b86aac2 to 322490e Compare August 7, 2020 04:16
@MaxDesiatov
Copy link
Contributor

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Aug 7, 2020

Build failed
Swift Test OS X Platform
Git Sha - 165af08

@swift-ci
Copy link
Contributor

swift-ci commented Aug 7, 2020

Build failed
Swift Test Linux Platform
Git Sha - 165af08

@kateinoigakukun
Copy link
Member Author

kateinoigakukun commented Aug 7, 2020 •
edited
Loading

Hmm, it seems the failure is not related to my change 🤔 Would you mind to trigger CI again?

@MaxDesiatov
Copy link
Contributor

@swift-ci please test

@kateinoigakukun kateinoigakukun mentioned this pull request Aug 11, 2020
Closed
@compnerd
Copy link
Member

This seems reasonable to me, though, Id like to see if @brentdax or @DougGregor have any thoughts on the naming.

@kateinoigakukun
Copy link
Member Author

@brentdax @DougGregor If you have time, could you take a look at this PR?

@beccadax
Copy link
Contributor

I would use either .swiftmodulesummary or just .swiftsummary. The other files adjacent to a swiftmodule—.swiftdoc, .swiftinterface, .swiftsourceinfo—don't use a second dot.

I'm also wondering if "summary" is the right term to use—it's very vague. But if it's a term of art in the LTO world, it might be appropriate.

@kateinoigakukun
Copy link
Member Author

kateinoigakukun commented Aug 24, 2020 •
edited
Loading

@brentdax

I would use either .swiftmodulesummary or just .swiftsummary. The other files adjacent to a swiftmodule—.swiftdoc, .swiftinterface, .swiftsourceinfo—don't use a second dot.

Ok, I prefer .swiftmodulesummary.

I'm also wondering if "summary" is the right term to use—it's very vague. But if it's a term of art in the LTO world, it might be appropriate.

In LLVM thin LTO world, "summary" is often used.
https://clang.llvm.org/docs/ThinLTO.html

In ThinLTO mode, as with regular LTO, clang emits LLVM bitcode after the compile phase. The ThinLTO bitcode is augmented with a compact summary of the module. During the link step, only the summaries are read and merged into a combined summary index, which includes an index of function locations for later cross-module function importing. Fast and efficient whole-program analysis is then performed on the combined summary index.

@kateinoigakukun kateinoigakukun force-pushed the katei/swift-module-summary/frontend branch 2 times, most recently from b868daf to c44904b Compare August 24, 2020 23:45
@kateinoigakukun
Copy link
Member Author

Updated to use . swiftmodulesummary instead of .swiftmodule.summary

beccadax
beccadax requested changes Aug 25, 2020
View reviewed changes
Copy link
Contributor

@beccadax beccadax left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm requesting some tiny changes, but these are basically just formatting things. Functionally, your PR looks fine.

include/swift/Basic/FileTypes.def Outdated
@@ -51,6 +51,7 @@ TYPE("swiftmodule", SwiftModuleFile, "swiftmodule", "")
TYPE("swiftdoc", SwiftModuleDocFile, "swiftdoc", "")
TYPE("swiftinterface", SwiftModuleInterfaceFile, "swiftinterface", "")
TYPE("private-swiftinterface", PrivateSwiftModuleInterfaceFile, "private.swiftinterface", "")
TYPE("swiftmodulesummary", SwiftModuleSummaryFile, "swiftmodulesummary", "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: You'll need an additional space here to fix the alignment of the columns.

include/swift/Option/Options.td
@@ -454,6 +454,17 @@ def emit_module_interface_path :
ArgumentIsPath, SupplementaryOutput]>,
MetaVarName<"<path>">, HelpText<"Output module interface file to <path>">;

def emit_module_summary :
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I would put these new entries either above emit_module_interface or below emit_private_module_interface_path to avoid splitting the module interface options from each other.

lib/FrontendTool/FrontendTool.cpp
auto summaryOutputPath = PSPs.SupplementaryOutputs.ModuleSummaryOutputPath;
return withOutputFile(Context.Diags, summaryOutputPath,
[&](llvm::raw_ostream &out) {
out << "Some stuff";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just quickly checking: if the tools that are meant to consume these files read Some stuff, will they break in an obvious way? (I don't know what you eventually intend for these files to look like.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, swiftmodulesummary will be serialized into LLVM bitcode stream, so if deserializer reads this text, it will fail due to signature checking.

@kateinoigakukun kateinoigakukun force-pushed the katei/swift-module-summary/frontend branch from c44904b to b0a2bb6 Compare August 25, 2020 00:11
@MaxDesiatov

This comment has been minimized.

Sign in to view
@kateinoigakukun kateinoigakukun force-pushed the katei/swift-module-summary/frontend branch from b0a2bb6 to 4716890 Compare August 26, 2020 07:33
@MaxDesiatov

This comment has been minimized.

Sign in to view
@MaxDesiatov MaxDesiatov requested a review from beccadax August 26, 2020 08:11
@MaxDesiatov

This comment has been minimized.

Sign in to view
@kateinoigakukun kateinoigakukun force-pushed the katei/swift-module-summary/frontend branch from 4716890 to 2930fe7 Compare August 26, 2020 14:38
@MaxDesiatov

This comment has been minimized.

Sign in to view
@kateinoigakukun
[Frontend] Set up output file .swiftmodule.summary
43fb346 
This patch focus on teaching frontend and driver to emit this file.
The actual serialization and deserialization implementation will come
later.
@kateinoigakukun kateinoigakukun force-pushed the katei/swift-module-summary/frontend branch from 2930fe7 to 43fb346 Compare August 26, 2020 15:41
@MaxDesiatov

This comment has been minimized.

Sign in to view
@MaxDesiatov

This comment has been minimized.

Sign in to view
1 similar comment
@MaxDesiatov

This comment has been minimized.

Sign in to view
@MaxDesiatov
Copy link
Contributor

Hi @brentdax, would you be able to have another look at this PR? Thank you!

beccadax
beccadax approved these changes Sep 7, 2020
View reviewed changes
Copy link
Contributor

@beccadax beccadax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Happy merging!

@MaxDesiatov MaxDesiatov merged commit 9685179 into swiftlang:master Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compnerd compnerd compnerd approved these changes

@beccadax beccadax beccadax approved these changes

@gottesmm gottesmm Awaiting requested review from gottesmm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kateinoigakukun @MaxDesiatov @swift-ci @compnerd @beccadax