Page MenuHomePhabricator

[LTO] Use lto::backend for code generation (WIP).
Needs ReviewPublic

Authored by fhahn on Tue, Jan 12, 3:25 AM.

Details

Summary

This patch updates LTOCodeGenerator to use the utilities provided by
LTOBackend to run middle-end optimizations and backend code generation.

This is a first step towards unifying the code used by libLTO's C API
and the newer, C++ interface (see PR41541).

The immediate motivation is to allow using the new pass manager when
doing LTO using libLTO's C API, which is used on Darwin, among others.

This is still WIP, as some of the error handling still needs to be
updated/improved, but I wanted to share it early, to get some more
high-level feedback.

Also, some options are not yet supported via lto::Config.

With the changes, there are no codegen/stats differences when building
MultiSource/SPEC2000/SPEC2006 on X86 with LTO, compared to without the
patch.

Diff Detail

Event Timeline

fhahn created this revision.Tue, Jan 12, 3:25 AM
fhahn requested review of this revision.Tue, Jan 12, 3:25 AM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: sstefan1. · View Herald Transcript

Thanks, I'll take a look hopefully today. As I just mentioned in the bug, ThinLTOCodeGenerator has the same issue - will you be migrating that as well?

Haven't looked at the detailed changes yet, but a couple of high level comments/questions below.

llvm/lib/LTO/LTOCodeGenerator.cpp
286

Looks like there is a remaining call to this compileOptimized from llvm-lto.cpp. Should that be switched and compileOptimized removed? Without it left in llvm-lto.cpp, I'm wondering if the removal of initializeLTOPasses above will be an issue since llvm-lto.cpp is still invoking the old PM.

291

Note that backend() when not called in CodeGenOnly mode will pretty much just call opt() and codegen(). With some more restructuring you wouldn't need separate calls to opt() from optimize() and backend with CodeGenOnly=1 here, but rather a single call to backend. Maybe just add a FIXME or other note to that effect here? Because it seems like longer term some of this could be cleaned up.

fhahn added a comment.Tue, Jan 12, 8:47 AM

Thanks, I'll take a look hopefully today. As I just mentioned in the bug, ThinLTOCodeGenerator has the same issue - will you be migrating that as well?

Yes, I can do that as well, once LTOCodeGenerator.cpp is wrapped up.

llvm/include/llvm/LTO/LTO.h
199

this could do with a comment. The changes here are because the existing uses allocate a new raw_pwrite_stream in a closure, and NativeObjectStream owns that. but in LTOCodeGenerator, we already opened the stream earlier, so we just have a regular pointer.

llvm/lib/LTO/LTOCodeGenerator.cpp
285

I'll change that to just return.

291

Because it seems like longer term some of this could be cleaned up.

Agreed, I think the long-term plan is to further unify/simplify the code. But given that the test coverage of the code is relatively limited, I think it would be safest if we move in small, incremental steps, to limit any breakage and make it easier to track down issues.

ormris added a subscriber: ormris.Tue, Jan 12, 9:07 AM

I think this is good to be a first step. The approach is correct and left some small suggestion above. The linter warnings need to be fixed as well.

Thanks for working on this.

llvm/lib/LTO/LTOCodeGenerator.cpp
134

You need to remove the function declaration in include/llvm/LTO/legacy/LTOCodeGenerator.h

588–589

I will try to remove all legacy pass manager from LTOCodegenerator in one commit to avoid complication. You can do ThinLTOCodeGenerator in a separate commit.

fhahn added inline comments.Wed, Jan 13, 1:05 PM
llvm/lib/LTO/LTOCodeGenerator.cpp
588–589

I think for that it might be best to expose the extension registration mechanism via lto::Config, so we can just configure it so it gets added to the pass manager used by splitCodegenand do not have to use a separate pass manager here?

fhahn added inline comments.Thu, Jan 14, 12:50 AM
llvm/lib/LTO/LTOCodeGenerator.cpp
588–589

There's a patch that tackles this: D94547

fhahn updated this revision to Diff 318502.Fri, Jan 22, 5:47 AM

Rebased after splitting off a few more changes, updated code to create lto::Config object.