This is an archive of the discontinued LLVM Phabricator instance.

[LTO][GlobalDCE] Use pass parameter instead of module flag for LTO phase
ClosedPublic

Authored by tejohnson on Jun 23 2023, 11:43 AM.

Details

Summary

D63932 added a module flag to indicate that we are executing the regular
LTO post merge pipeline, so that GlobalDCE could perform more aggressive
optimization for Dead Virtual Function Elimination. This caused issues
trying to reuse bitcode that had already been through the LTO pipeline
(see context in D139816).

Instead support this by passing down a parameter flag to the
GlobalDCEPass constructor, which is the more usual way for indicating
this information.

Most test changes are to remove incidental uses of this flag. Of the 2
real uses, llvm/test/LTO/ARM/lto-linking-metadata.ll is now obsolete and
removed in this patch, and the virtual-functions-visibility-post-lto.ll
test is updated to use the regular LTO default pipeline where this
parameter is set to true.

Diff Detail

Unit TestsFailed

Event Timeline

tejohnson created this revision.Jun 23 2023, 11:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2023, 11:43 AM
tejohnson requested review of this revision.Jun 23 2023, 11:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2023, 11:43 AM
Herald added a subscriber: wdng. · View Herald Transcript
nikic added a subscriber: nikic.

You need to also implement printing and parsing support for the new pass option.

You need to also implement printing and parsing support for the new pass option.

(https://reviews.llvm.org/D153437 as an example)

You need to also implement printing and parsing support for the new pass option.

(https://reviews.llvm.org/D153437 as an example)

Ah missed that. Done.

I also updated new-pm-print-pipeline.ll to test the new parameter.

Expanded virtual-functions-visibility-post-lto.ll test to use globaldce with the new parameter in addition to testing by invoking the full regular LTO pass pipeline. I also expanded this to ensure the transformation doesn't occur if we don't specify the new parameter (or use a non-lto default pipeline).

tejohnson updated this revision to Diff 534077.Jun 23 2023, 2:37 PM

Address comments

aeubanks accepted this revision.Jun 23 2023, 2:44 PM

lgtm with one comment, this is much cleaner

llvm/lib/Passes/PassBuilder.cpp
675 ↗(On Diff #534077)

GlobalDCE

This revision is now accepted and ready to land.Jun 23 2023, 2:44 PM
arsenm added inline comments.Jun 23 2023, 2:45 PM
llvm/lib/Passes/PassBuilder.cpp
675 ↗(On Diff #534077)

Can you use a name descriptive of the behavior change, rather than the why/where?

tejohnson marked 2 inline comments as done.Jun 23 2023, 2:51 PM
tejohnson added inline comments.
llvm/lib/Passes/PassBuilder.cpp
675 ↗(On Diff #534077)

Looks like it is supposed to be the pass name, fixed.

tejohnson updated this revision to Diff 534082.Jun 23 2023, 2:51 PM
tejohnson marked an inline comment as done.

Address comments

arsenm added inline comments.Jun 23 2023, 2:53 PM
llvm/lib/Passes/PassBuilder.cpp
675 ↗(On Diff #534077)

I mean "in-lto-post-link" doesn't tell me how the pass is going to behave differently. I mean something like handle-vtable-something or skip-something

tejohnson added inline comments.Jun 23 2023, 3:05 PM
llvm/lib/Passes/PassBuilder.cpp
675 ↗(On Diff #534077)

Oh hmm, I was thought this should be the same as the parameter name, which I was trying to keep close to the previous module flag.

I'm not sure what to rename this to that is descriptive yet not too long. The behavior is basically: when in the LTO post link, VFE can be applied to vtables with linkage unit vcall visibility (by default it is just those with translation unit vcall visibility). Open to suggestions.

arsenm added inline comments.Jun 23 2023, 3:19 PM
llvm/lib/Passes/PassBuilder.cpp
675 ↗(On Diff #534077)

After skimming the code and barely knowing anything about this optimization, my suggestions:

'assume-visible-vcall-table'
'assume-vcall-visibility-translation-unit'
'assume-module-visibility'
'whole-module-vfe'

tejohnson added inline comments.Jun 23 2023, 3:24 PM
llvm/lib/Passes/PassBuilder.cpp
675 ↗(On Diff #534077)

Ok thanks. After considering these and thinking about it some more, I've settled on 'vfe-linkage-unit-visibility' (since we are telling VFE that it can assume it has visibility over the full linkage unit).

tejohnson updated this revision to Diff 534094.Jun 23 2023, 3:25 PM

Change pass option string name

MaskRay accepted this revision.Jun 23 2023, 3:36 PM
arsenm accepted this revision.Jun 23 2023, 3:38 PM
tejohnson updated this revision to Diff 534127.Jun 23 2023, 5:04 PM

Fix one place I forgot to update with the new name

This revision was landed with ongoing or failed builds.Jun 23 2023, 5:05 PM
This revision was automatically updated to reflect the committed changes.
llvm/test/DebugInfo/X86/subprogram-across-cus.ll