-
Notifications
You must be signed in to change notification settings - Fork 15.5k
[Flang][Driver] Emit module summary for Full LTO #164302
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
Conversation
|
@llvm/pr-subscribers-flang-driver Author: Anchu Rajendran S (anchuraj) ChangesThis PR enables module summary for LTO. Information in module summary is used in performing LTO especially in Thin LTO. Full diff: https://github.com/llvm/llvm-project/pull/164302.diff 3 Files Affected:
diff --git a/flang/lib/Frontend/FrontendActions.cpp b/flang/lib/Frontend/FrontendActions.cpp
index 0c630d2ba876d..0ff5ba17ca874 100644
--- a/flang/lib/Frontend/FrontendActions.cpp
+++ b/flang/lib/Frontend/FrontendActions.cpp
@@ -1019,11 +1019,15 @@ void CodeGenAction::runOptimizationPipeline(llvm::raw_pwrite_stream &os) {
// Create the pass manager.
llvm::ModulePassManager mpm;
+ // The module summary should be emitted by default for regular LTO
+ // except for ld64 targets.
+ bool emitSummary = (opts.PrepareForThinLTO || opts.PrepareForFullLTO) &&
+ (triple.getVendor() != llvm::Triple::Apple);
+
+ if (emitSummary && !opts.PrepareForThinLTO)
+ llvmModule->addModuleFlag(llvm::Module::Error, "ThinLTO", uint32_t(0));
+
if (opts.PrepareForFatLTO) {
- // The module summary should be emitted by default for regular LTO
- // except for ld64 targets.
- bool emitSummary = opts.PrepareForThinLTO || opts.PrepareForFullLTO ||
- triple.getVendor() != llvm::Triple::Apple;
mpm = pb.buildFatLTODefaultPipeline(level, opts.PrepareForThinLTO,
emitSummary);
} else if (opts.PrepareForFullLTO)
@@ -1034,9 +1038,9 @@ void CodeGenAction::runOptimizationPipeline(llvm::raw_pwrite_stream &os) {
mpm = pb.buildPerModuleDefaultPipeline(level);
if (action == BackendActionTy::Backend_EmitBC)
- mpm.addPass(llvm::BitcodeWriterPass(os));
+ mpm.addPass(llvm::BitcodeWriterPass(os, false, emitSummary));
else if (action == BackendActionTy::Backend_EmitLL)
- mpm.addPass(llvm::PrintModulePass(os));
+ mpm.addPass(llvm::PrintModulePass(os, "", false, emitSummary));
// FIXME: This should eventually be replaced by a first-class driver option.
// This should be done for both flang and clang simultaneously.
diff --git a/flang/test/CMakeLists.txt b/flang/test/CMakeLists.txt
index da557f9ec3443..8c8e92faa787a 100644
--- a/flang/test/CMakeLists.txt
+++ b/flang/test/CMakeLists.txt
@@ -72,6 +72,7 @@ if (NOT FLANG_STANDALONE_BUILD)
FileCheck
count
not
+ llvm-bcanalyzer
llvm-dis
llvm-objcopy
llvm-objdump
diff --git a/flang/test/Driver/lto-bc.f90 b/flang/test/Driver/lto-bc.f90
index 5e34cdb87c5b1..fbf964b849eb7 100644
--- a/flang/test/Driver/lto-bc.f90
+++ b/flang/test/Driver/lto-bc.f90
@@ -1,21 +1,35 @@
! Test that the output is LLVM bitcode for LTO and not a native objectfile by
-! disassembling it to LLVM IR.
-! Right now there is nothing special about it and it is similar to non-lto IR,
-! more work is needed to add things like module summaries.
+! disassembling it to LLVM IR. Also tests module summaries are emitted for LTO
! RUN: %flang %s -c -o - | not llvm-dis -o %t
! RUN: %flang_fc1 %s -emit-llvm-bc -o - | llvm-dis -o - | FileCheck %s
-
-! RUN: %flang -flto %s -c -o - | llvm-dis -o - | FileCheck %s
-! RUN: %flang -flto=thin %s -c -o - | llvm-dis -o - | FileCheck %s
-
! CHECK: define void @_QQmain()
! CHECK-NEXT: ret void
! CHECK-NEXT: }
+! CHECK-NOT: !{{.*}} = !{i32 1, !"ThinLTO", i32 0}
+! CHECK-NOT: ^{{.*}} = module:
+! CHECK-NOT: ^{{.*}} = gv: (name:
+! CHECK-NOT: ^{{.*}} = blockcount:
+
+! RUN: %flang -flto %s -c -o - | llvm-dis -o - | FileCheck %s --check-prefix=FULL
+! THIN: define void @_QQmain()
+! THIN-NEXT: ret void
+! THIN-NEXT: }
+! THIN-NOT: !{{.*}} = !{i32 1, !"ThinLTO", i32 0}
+! THIN: ^{{.*}} = module:
+! THIN: ^{{.*}} = gv: (name:
+! THIN: ^{{.*}} = blockcount:
-! CHECK-NOT: ^0 = module:
-! CHECK-NOT: ^1 = gv: (name:
-! CHECK-NOT: ^2 = flags:
-! CHECK-NOT: ^3 = blockcount:
+! RUN: %flang -flto=thin %s -c -o - | llvm-dis -o - | FileCheck %s --check-prefix=THIN
+! FULL: define void @_QQmain()
+! FULL-NEXT: ret void
+! FULL-NEXT: }
+! FULL: !{{.*}} = !{i32 1, !"ThinLTO", i32 0}
+! FULL: ^{{.*}} = module:
+! FULL: ^{{.*}} = gv: (name:
+! FULL: ^{{.*}} = blockcount:
+! RUN: %flang_fc1 -flto -emit-llvm-bc %s -o - | llvm-bcanalyzer -dump| FileCheck --check-prefix=MOD-SUMM %s
+! MOD-SUMM: FULL_LTO_GLOBALVAL_SUMMARY_BLOCK
+program main
end program
|
8d5366a to
3309893
Compare
| ! THIN: define void @_QQmain() | ||
| ! THIN-NEXT: ret void | ||
| ! THIN-NEXT: } | ||
| ! THIN-NOT: !{{.*}} = !{i32 1, !"ThinLTO", i32 0} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When setting -flto=full, metadata named ThinLTO is present in the module (line 27), but when -flto=thin, it is not? Does the 0 value for "ThinLTO" mean that the ThinLTO is off?
Also, it looks like the output in both the thin and full LTO cases are the same - or at least, what is being checked in this test is the same. What is the difference between the two schemes?
I apologize for my ignorance - as I said, I am not very familiar with LLVM's LTO implementation. Do you mind clarifying some of this? Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification (in the response to a separate comment)
To be clear, if testing the output of full vs. thin is too much, we need not do it - presumably that will have been tested elsewhere, perhaps in llvm/LTO. But if we can test something other than the presence/absence of "ThinLTO" metadata, we probably should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Working on these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, ThinLTO requires more work to enable module summaries. I was wrong to test only the emitted module summary. At least https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/BackendUtil.cpp#L1168C9-L1182C8 needs to be enabled. I have removed the tests for thinLTO. This PR enables only module summary for Full LTO by default (same as https://reviews.llvm.org/D34156). Thank you for pointing out this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this, will -flto=thin and -flto=full produce the same output? Or will the output of -flto=thin be the same as the output if a -flto= option was not given at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, flto = thin does not emit the summary, only flto = full emits. Thin LTO summary emission logic does not change - it is same as previous:
if (action == BackendActionTy::Backend_EmitBC)
mpm.addPass(llvm::BitcodeWriterPass(os));
else if (action == BackendActionTy::Backend_EmitLL)
mpm.addPass(llvm::PrintModulePass(os));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flang hello.f90 -c -emit-llvm -o -| llvm-dis -o - and flang -flto=thin hello.f90 -c -o -| llvm-dis -o - will produce the same output in ll files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I got a bit confused and asked a strange question. Let me try again.
In this test, when -flto=full, we are checking for the "ThinLTO = 0" metadata and the presence of "module:, gv:, blockcount:" (I assume that this is the summary, is that right?).
However, when flto=thin, we are only checking for the absence of the "ThinLTO" metadata. We are not checking for the absence of the summaries. Should we be doing that as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is the module summary. Thanks a lot for reviewing this and helping me to improve my change. Since I am learning through this as well, there are misses which I try to correct. As this is a work in progress and as is a feature not yet implemented, I did not have preferences for checking or not checking this. I have added it back since it is more consistent with the existed test case.
139bc9b to
7ed30f8
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
7ed30f8 to
51e02c1
Compare
| triple.getVendor() != llvm::Triple::Apple; | ||
| // The module summary should be emitted by default for regular LTO | ||
| // except for ld64 targets. | ||
| bool emitSummary = (opts.PrepareForThinLTO || opts.PrepareForFullLTO) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since ThinLTO support is not being enabled here, should emitSummary only be true if opt.PrepareForFullLTO is set? Should PrepareForThinLTO be ignored here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary should be emitted for thinLTO. I will work on this as a follow upThe flag set logic is correct. However it is not used for thin LTO right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that since emitSummary is ignored in the if (opts.PrepareForThinLTO) branch on line 1038, the code should look like this for now:
bool emitSummary = opts.PrepareForFullLTO &&
triple.getVendor() != llvm::Triple::Apple;
When support for summaries with ThinLTO is implemented, it can become:
bool emitSummary = (opts.PrepareForThinLTO || opts.PrepareForFullLTO) &&
triple.getVendor() != llvm::Triple::Apple;
However, since you intend to work on enabling summaries for thin LTO right away (if I understand you correctly), it should be ok to leave this code as it is. Otherwise, it can be a bit confusing.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Agree that it can be confusing.
| if (action == BackendActionTy::Backend_EmitBC || | ||
| action == BackendActionTy::Backend_EmitLL || opts.PrepareForFatLTO) { | ||
| if (opts.PrepareForThinLTO) { | ||
| // TODO: ThinLTO module summary support is yet to be enabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does -flto=thin imply that module summaries are enabled? If so, it may be a good idea to issue a diagnostic saying that thin-lto support is incomplete. Do you intend to implement module summaries for thin-lto? If that is likely to land relatively soon, it may be ok not to emit a diagnostic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning exists: https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/lto-flags.f90#L33 I will work on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this
| ! THIN: define void @_QQmain() | ||
| ! THIN-NEXT: ret void | ||
| ! THIN-NEXT: } | ||
| ! THIN-NOT: !{{.*}} = !{i32 1, !"ThinLTO", i32 0} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this, will -flto=thin and -flto=full produce the same output? Or will the output of -flto=thin be the same as the output if a -flto= option was not given at all?
tarunprabhu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my comments. Unfortunately, I was not at all clear, and I have tried again. Thanks for your patience.
| triple.getVendor() != llvm::Triple::Apple; | ||
| // The module summary should be emitted by default for regular LTO | ||
| // except for ld64 targets. | ||
| bool emitSummary = (opts.PrepareForThinLTO || opts.PrepareForFullLTO) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that since emitSummary is ignored in the if (opts.PrepareForThinLTO) branch on line 1038, the code should look like this for now:
bool emitSummary = opts.PrepareForFullLTO &&
triple.getVendor() != llvm::Triple::Apple;
When support for summaries with ThinLTO is implemented, it can become:
bool emitSummary = (opts.PrepareForThinLTO || opts.PrepareForFullLTO) &&
triple.getVendor() != llvm::Triple::Apple;
However, since you intend to work on enabling summaries for thin LTO right away (if I understand you correctly), it should be ok to leave this code as it is. Otherwise, it can be a bit confusing.
What do you think?
| ! THIN: define void @_QQmain() | ||
| ! THIN-NEXT: ret void | ||
| ! THIN-NEXT: } | ||
| ! THIN-NOT: !{{.*}} = !{i32 1, !"ThinLTO", i32 0} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I got a bit confused and asked a strange question. Let me try again.
In this test, when -flto=full, we are checking for the "ThinLTO = 0" metadata and the presence of "module:, gv:, blockcount:" (I assume that this is the summary, is that right?).
However, when flto=thin, we are only checking for the absence of the "ThinLTO" metadata. We are not checking for the absence of the summaries. Should we be doing that as well?
a6dd62b to
ee40244
Compare
tarunprabhu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience and for the changes.
This PR enables module summary for Full LTO. Module summaries are enabled by default for Full LTO in clang, this change makes the flang behaviour consistent. Reference PR: https://reviews.llvm.org/D34156
This PR enables module summary for Full LTO. Module summaries are enabled by default for Full LTO in clang, this change makes the flang behaviour consistent. Reference PR: https://reviews.llvm.org/D34156
This PR enables module summary for Full LTO. Module summaries are enabled by default for Full LTO in clang, this change makes the flang behaviour consistent. Reference PR: https://reviews.llvm.org/D34156
This PR enables module summary for Full LTO. Module summaries are enabled by default for Full LTO in clang, this change makes the flang behaviour consistent. Reference PR: https://reviews.llvm.org/D34156