Page MenuHomePhabricator

[WIP][llvm] A Unified LTO Bitcode Frontend
Needs ReviewPublic

Authored by ormris on Apr 14 2022, 9:39 AM.

Details

Summary

Here's a high level summary of the changes in this patch. For more information
on rational, see the RFC (https://discourse.llvm.org/t/rfc-a-unified-lto-bitcode-frontend/61774).

  • Add config parameter to LTO backend, specifying which LTO mode is desired when using unified LTO.
  • Add unified LTO flag to the summary index for efficiency. Unified LTO modules can be detected without parsing the module.
  • Make sure that the ModuleID is generated by incorporating more types of symbols.

Diff Detail

Event Timeline

ormris created this revision.Apr 14 2022, 9:39 AM
ormris requested review of this revision.Apr 14 2022, 9:39 AM
ormris edited the summary of this revision. (Show Details)Apr 14 2022, 9:48 AM
ormris edited the summary of this revision. (Show Details)Apr 14 2022, 10:16 AM
ormris edited the summary of this revision. (Show Details)
ychen added a subscriber: ychen.Apr 14 2022, 11:06 AM
mehdi_amini added inline comments.Apr 15 2022, 10:43 AM
llvm/lib/Passes/PassBuilder.cpp
1144

It is concerning to me that we add one mode different code path / behavior to maintain instead of unifying everything.

If UnifiedLTO is able to use the LTO pipeline effectively, what would be the reason for ThinLTO to not align?

tejohnson added inline comments.Apr 15 2022, 11:23 AM
llvm/lib/Passes/PassBuilder.cpp
1144

If UnifiedLTO is able to use the LTO pipeline effectively, what would be the reason for ThinLTO to not align?

Perhaps it can eventually, but I would not want to make a major change to the ThinLTO pipelines without a lot of experimentation. I don't personally have the bandwidth to do that right now, but if this was in as an alternative mode under an option, it could be done more easily at some point on a wider range of applications. I'd be concerned for example of side effects on importing behavior which is based on instruction count thresholds.

mehdi_amini added inline comments.Apr 15 2022, 11:27 AM
llvm/lib/Passes/PassBuilder.cpp
1144

Right, but your objection is exactly the root of my concerned with this new mode in the first place right now.

tejohnson added inline comments.Apr 15 2022, 11:41 AM
llvm/lib/Passes/PassBuilder.cpp
1144

Yeah, it isn't ideal to have added complexity, but I do understand the different constraints. The new mode seems to work well enough for Sony's needs, but for users such as mine at Google that want to maximize performance from ThinLTO, it may not be the best approach (or may be ok, but needs to be carefully evaluated). Unfortunately, I don't have a good immediate solution to balancing those two sets of needs at the moment, other than supporting different modes.

I wonder if we can get partly to a more common approach but just have a flag to switch between the different pass managers in the pre and post LTO optimization pipelines. I haven't had a chance to look closely at the patches yet, but my sense is that the other major change is enabling "split" LTO bitcode files always, for which I don't yet have a good understanding of the implications. I'll try to spend some time looking at the patches in more detail in the next few days.

tejohnson added inline comments.Apr 15 2022, 1:46 PM
llvm/lib/Passes/PassBuilder.cpp
1144

Per discussion on the RFC, the unified LTO mode added here requires split thin/regular LTO units. This is not something we have been able to use internally because of the scalability of the regular LTO portions. So we will need to keep the usual "pure" ThinLTO mode operational.

tejohnson added inline comments.Apr 18 2022, 10:05 AM
llvm/lib/LTO/LTO.cpp
739

Why is this needed?

1128

Why is this needed?

1148

Why is this needed?

1547

Why is this needed?

llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
212

Rather than adding the many checks in the file below, can the Perform* and PrepareFor* options just be initialized differently under the UnifiedLTO mode?

llvm/lib/Transforms/Utils/ModuleUtils.cpp
225

As noted elsewhere, I'd prefer the UnifiedLTO not imply splitting. But regardless, this seems like a useful change whenever LTO unit is enabled - why not always compute the module hash this way?

ormris added inline comments.Apr 18 2022, 1:38 PM
llvm/lib/LTO/LTO.cpp
739

If cfi.functions isn't removed, LowerTypeTests will rename local functions in the merged module as "<function name>.1" when the regular LTO backend is used. This causes linking errors, since other parts of the module expect the original function name. We saw this happen in internal testing.

1128

Looks like it's not needed. I'll remove it.

llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
212

No, I don't think so. The UnifiedLTO flag doesn't match any of those variables. I don't see a combination Perform*/PrepareFor* that would cleanly produce the result we want. I would also worry that reusing these variables would make this code less clear. Looking at it now, I wonder if it should be called PrepareForUnifiedLTO, though.