Page MenuHomePhabricator

[LTO] Use lto::backend for code generation.
ClosedPublic

Authored by fhahn on Jan 12 2021, 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.

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

Diff Detail

Event Timeline

fhahn created this revision.Jan 12 2021, 3:25 AM
fhahn requested review of this revision.Jan 12 2021, 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.

290

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.Jan 12 2021, 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 ↗(On Diff #316044)

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
284

I'll change that to just return.

290

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.Jan 12 2021, 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

582–583

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.Jan 13 2021, 1:05 PM
llvm/lib/LTO/LTOCodeGenerator.cpp
582–583

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.Jan 14 2021, 12:50 AM
llvm/lib/LTO/LTOCodeGenerator.cpp
582–583

There's a patch that tackles this: D94547

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

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

fhahn updated this revision to Diff 319554.Jan 27 2021, 6:32 AM

Rebase after recent changes to D95222

fhahn retitled this revision from [LTO] Use lto::backend for code generation (WIP). to [LTO] Use lto::backend for code generation..Jan 28 2021, 8:23 AM
fhahn edited the summary of this revision. (Show Details)
fhahn updated this revision to Diff 319881.Jan 28 2021, 8:24 AM

Rebased after all required patches except D95222 landed. All original issues should have been addressed now, so it is no longer WIP.

LGTM with only one comment.

llvm/include/llvm/LTO/LTO.h
202 ↗(On Diff #319881)

I guess this only exists to support bool compileOptimized(ArrayRef<raw_pwrite_stream *> Out)?

That seems to only an interface used in llvm-lto which is a test tool. I wonder if we can just change the interface so we don't need to worry about if NativeObjectStream owning the output stream or not.

dexonsmith added inline comments.Jan 29 2021, 11:04 AM
llvm/include/llvm/LTO/LTO.h
198 ↗(On Diff #319881)

As a drive-by comment: if this sticks around, please make construct this from a reference to prevent raw owned pointers from coming in this way (if we need a constructor from nullptr the unique_ptr constructor will still work). (That said, I agree with Steven that changing the test tool would be better.)

fhahn updated this revision to Diff 320213.Jan 29 2021, 2:12 PM

Update compileOptimized to take AddStream callback and parallelism level, instead of outstream pointers.

fhahn updated this revision to Diff 320214.Jan 29 2021, 2:15 PM

Remove stray newline

fhahn updated this revision to Diff 320215.Jan 29 2021, 2:22 PM

Update llvm-lto to preserve existing behavior for -o -, undo unnecessary test changes;

llvm/include/llvm/LTO/LTO.h
198 ↗(On Diff #319881)

Thanks, the code is gone now

202 ↗(On Diff #319881)

I guess this only exists to support bool compileOptimized(ArrayRef<raw_pwrite_stream *> Out)?

There's a another users in LTOCodegenerator, but that can also be updated. I changed to interface to accept and AddStream callback and the parallelism level. This code is not longer needed.

steven_wu accepted this revision.Jan 29 2021, 2:24 PM

This looks clean. LGTM. ThinLTO change in a separate commit?

This revision is now accepted and ready to land.Jan 29 2021, 2:24 PM
Harbormaster completed remote builds in B87209: Diff 320215.
This revision was landed with ongoing or failed builds.Jan 30 2021, 3:18 AM
This revision was automatically updated to reflect the committed changes.
fhahn added a comment.Jan 30 2021, 8:28 AM

Thanks everyone!

This looks clean. LGTM. ThinLTO change in a separate commit?

I am planning on first moving/re-using as much from LTO.cpp and then take a look at ThinLTO