Skip to content

[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.

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

Merged
anchuraj merged 5 commits into from
Oct 23, 2025
Merged

[Flang][Driver] Emit module summary for Full LTO #164302

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3309893
[Flang][Driver] Emit module summary for LTO
anchuraj Oct 17, 2025
1ecbeb9
R2: Addressing review comments
anchuraj Oct 21, 2025
51e02c1
Removing the changes from ThinLTO
anchuraj Oct 21, 2025
c050671
R3: Addressing review comments
anchuraj Oct 22, 2025
ee40244
R4: Addressing review comments
anchuraj Oct 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions flang/lib/Frontend/FrontendActions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) &&
Copy link
Contributor

@tarunprabhu tarunprabhu Oct 21, 2025

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?

Copy link
Contributor Author

@anchuraj anchuraj Oct 22, 2025 •
edited
Loading

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.

Copy link
Contributor

@tarunprabhu tarunprabhu Oct 22, 2025

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?

Copy link
Contributor Author

@anchuraj anchuraj Oct 22, 2025 •
edited
Loading

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.

(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)
Expand All @@ -1034,9 +1038,11 @@ 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, /*ShouldPreserveUseListOrder=*/false, emitSummary));
else if (action == BackendActionTy::Backend_EmitLL)
mpm.addPass(llvm::PrintModulePass(os));
mpm.addPass(llvm::PrintModulePass(
os, /*Banner=*/"", /*ShouldPreserveUseListOrder=*/false, emitSummary));

// FIXME: This should eventually be replaced by a first-class driver option.
// This should be done for both flang and clang simultaneously.
Expand Down
1 change: 1 addition & 0 deletions flang/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ if (NOT FLANG_STANDALONE_BUILD)
FileCheck
count
not
llvm-bcanalyzer
llvm-dis
llvm-objcopy
llvm-objdump
Expand Down
36 changes: 25 additions & 11 deletions flang/test/Driver/lto-bc.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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}
Copy link
Contributor

@tarunprabhu tarunprabhu Oct 21, 2025

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.

Copy link
Contributor

@tarunprabhu tarunprabhu Oct 21, 2025

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.

Copy link
Contributor Author

@anchuraj anchuraj Oct 21, 2025

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.

Copy link
Contributor Author

@anchuraj anchuraj Oct 21, 2025

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

Copy link
Contributor

@tarunprabhu tarunprabhu Oct 21, 2025

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?

Copy link
Contributor Author

@anchuraj anchuraj Oct 22, 2025 •
edited
Loading

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));

Copy link
Contributor Author

@anchuraj anchuraj Oct 22, 2025

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.

Copy link
Contributor

@tarunprabhu tarunprabhu Oct 22, 2025

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?

Copy link
Contributor Author

@anchuraj anchuraj Oct 22, 2025

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.

! 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
Loading