This is an archive of the discontinued LLVM Phabricator instance.

Differential D16173

[ThinLTO] Ensure prevailing linkonce emitted as weak in ThinLTO backends
ClosedPublic

Authored by tejohnson on Jan 13 2016, 8:00 PM.

Details

Reviewers
davidxl
• rafael
Commits
rGa17f2cd1a3d6: [ThinLTO] Ensure prevailing linkonce emitted as weak in ThinLTO backends
rL262727: [ThinLTO] Ensure prevailing linkonce emitted as weak in ThinLTO backends
Summary

Since IR files are all compiled into separate independent object files in ThinLTO mode,
the prevailing linkonce symbols must be emitted in its object file even if it is no longer
referenced there, e.g. if no references remain in the module after inlining, since it may
be referenced by another ThinLTO compiled object file. This is done by changing
LDPR_PREVAILING_DEF_IRONLY* symbols to LDPR_PREVAILING_DEF, which
converts the prevailing linkonce to weak. We also don't need the other prevailing
IRONLY handling for internalization, which is not currently performed for ThinLTO.

Test case included.

Diff Detail

Repository
rL LLVM

Event Timeline

tejohnson updated this revision to Diff 44828.Jan 13 2016, 8:00 PM
tejohnson retitled this revision from to [ThinLTO] Do not mark linkonce symbols as preempted in ThinLTO backends.
tejohnson updated this object.
tejohnson added reviewers: • rafael, davidxl.
tejohnson added a subscriber: llvm-commits.
Herald added a subscriber: mehdi_amini. · View Herald TranscriptJan 13 2016, 8:00 PM
tejohnson added a parent revision: D15390: [ThinLTO] Launch importing backends in parallel threads from gold plugin.Jan 13 2016, 8:09 PM
tejohnson mentioned this in D15390: [ThinLTO] Launch importing backends in parallel threads from gold plugin.Jan 13 2016, 8:15 PM
mehdi_amini added a comment.Jan 28 2016, 9:50 AM

It seems to me that you are changing the ODR resolution compared to a regular FullLTO where the %t2 copy of f() would be selected. I don't know how much it is a problem.

tejohnson added a comment.Jan 28 2016, 10:00 AM
In D16173#338468, @joker.eph wrote:

It seems to me that you are changing the ODR resolution compared to a regular FullLTO where the %t2 copy of f() would be selected. I don't know how much it is a problem.

Although it won't change it compared to a non-LTO link in the same order (where none would be preempted).

Rafael?

mehdi_amini added a comment.Jan 28 2016, 10:18 AM

Mmmm, not sure how to compare to a non-LTO link: both definition of f would remain and won't disappear.

• rafael edited edge metadata.Feb 16 2016, 12:31 PM

Is there any change the file that got the prevailing def could have the linkage changed to weak_odr? That way we would know it would output the symbol, using the same logic that is used in the regular lto case.

tejohnson added a comment.Feb 22 2016, 8:50 AM
In D16173#353811, @rafael wrote:

Is there any change the file that got the prevailing def could have the linkage changed to weak_odr? That way we would know it would output the symbol, using the same logic that is used in the regular lto case.

Yes, it seems like we could change the handling of LDPR_PREVAILING_DEF_IRONLY (and LDPR_PREVAILING_DEF_IRONLY_EXP I think) to be the same as LDPR_PREVAILING_DEF in the below switch statement. that would change linkonce_any/odr to weak_any/odr and keep the symbol. Let me try that and will update this patch.

tejohnson updated this revision to Diff 48757.Feb 22 2016, 4:55 PM
tejohnson edited edge metadata.

Implement Rafael's suggestion to convert a prevailing def that is
linkonce to weak instead of not marking linkonce as preempted.

tejohnson added a comment.Feb 22 2016, 5:02 PM

Please ignore this latest revision - it pulled in all the differences from the revision on which it was dependent (the gold thinlto threads patch) as well. I'll see if I can fix it, otherwise will kill this one and create a new patch.

tejohnson updated this revision to Diff 48760.Feb 22 2016, 5:11 PM

Implement Rafael's suggestion to convert a prevailing def that is
linkonce to weak instead of not marking linkonce as preempted.

tejohnson added a comment.Feb 22 2016, 5:11 PM

Ok, the diff is fixed now. PTAL. Thanks!

• rafael added a subscriber: • rafael.Feb 24 2016, 2:57 PM

The description is now out of sync.

• rafael accepted this revision.Feb 24 2016, 3:07 PM
• rafael edited edge metadata.

Just before the code you changed there is an area were we patch gold's decision. Would it work to just add

if (options::thinlto && (Resolution == LDPR_PREVAILING_DEF_IRONLY_EXP || Resolution == LDPR_PREVAILING_DEF_IRONLY))

Resolution =  LDPR_PREVAILING_DEF;

If so, I think that is better.

LGTM either way.

test/tools/gold/X86/thinlto_linkonceresolution.ll
14 ↗(On Diff #48760)

if you pass save-temps you should be able to more directly test the linkage change, no?

This revision is now accepted and ready to land.Feb 24 2016, 3:07 PM
tejohnson added a comment.Feb 25 2016, 9:11 AM
In D16173#361067, @rafael wrote:

Just before the code you changed there is an area were we patch gold's decision. Would it work to just add

if (options::thinlto && (Resolution == LDPR_PREVAILING_DEF_IRONLY_EXP || Resolution == LDPR_PREVAILING_DEF_IRONLY))

Resolution =  LDPR_PREVAILING_DEF;

If so, I think that is better.

LGTM either way.

I like your suggestion, it is cleaner. Implemented and will upload new patch here. Will also update description in Phab and in git before I commit. Note that this is dependent on the gold threading patch D15390 (for the test case to work), so I will wait to commit this until after that goes in (hopefully very very soon! =) ).

test/tools/gold/X86/thinlto_linkonceresolution.ll
14 ↗(On Diff #48760)

Good idea, adding that.

tejohnson updated this revision to Diff 49080.Feb 25 2016, 9:21 AM
tejohnson retitled this revision from [ThinLTO] Do not mark linkonce symbols as preempted in ThinLTO backends to [ThinLTO] Ensure prevailing linkonce emitted as weak in ThinLTO backends.
tejohnson updated this object.
tejohnson edited edge metadata.

Implement Rafael's suggestion to change resolution for all ThinLTO
prevailing copies to LDPR_PREVAILING_DEF, and modify test to check
linkage type directly with save-temps.

Update description/title.

Closed by commit rL262727: [ThinLTO] Ensure prevailing linkonce emitted as weak in ThinLTO backends (authored by tejohnson). · Explain WhyMar 4 2016, 9:53 AM
This revision was automatically updated to reflect the committed changes.
tejohnson mentioned this in D18346: ThinLTO: special handling for LinkOnce functions.Apr 1 2016, 12:51 PM

Revision Contents

PathSize
llvm/
trunk/
test/
tools/
gold/
X86/
Inputs/
4 lines
32 lines
tools/
gold/
10 lines

Diff 49838

llvm/trunk/test/tools/gold/X86/Inputs/thinlto_linkonceresolution.ll

target triple = "x86_64-unknown-linux-gnu"
define linkonce_odr hidden void @f() {
ret void
}

llvm/trunk/test/tools/gold/X86/thinlto_linkonceresolution.ll

; RUN: llvm-as -function-summary %s -o %t.o
; RUN: llvm-as -function-summary %p/Inputs/thinlto_linkonceresolution.ll -o %t2.o
; Ensure the plugin ensures that for ThinLTO the prevailing copy of a
; linkonce symbol is changed to weak to ensure it is not eliminated.
; Note that gold picks the first copy of f() as the prevailing one,
; so listing %t2.o first is sufficient to ensure that this copy is
; preempted. Also, set the import-instr-limit to 0 to prevent f() from
; being imported from %t2.o which hides the problem.
; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
; RUN: --plugin-opt=thinlto \
; RUN: --plugin-opt=-import-instr-limit=0 \
; RUN: --plugin-opt=save-temps \
; RUN: -o %t3.o %t2.o %t.o
; RUN: llvm-nm %t3.o | FileCheck %s
; RUN: llvm-dis %t.o.opt.bc -o - | FileCheck --check-prefix=OPT %s
; RUN: llvm-dis %t2.o.opt.bc -o - | FileCheck --check-prefix=OPT2 %s
; Ensure that f() is defined in resulting object file, and also
; confirm the weak linkage directly in the saved opt bitcode files.
; CHECK-NOT: U f
; OPT: declare extern_weak hidden void @f()
; OPT2: define weak_odr hidden void @f()
target triple = "x86_64-unknown-linux-gnu"
define i32 @g() {
call void @f()
ret i32 0
}
define linkonce_odr hidden void @f() {
ret void
}

llvm/trunk/tools/gold/gold-plugin.cpp

Show First 20 Lines • Show All 703 Lines • ▼ Show 20 Lines if (!GV) {
freeSymName(Sym); freeSymName(Sym);
continue; // Asm symbol. continue; // Asm symbol.
} }
ResolutionInfo &Res = ResInfo[Sym.name]; ResolutionInfo &Res = ResInfo[Sym.name];
if (Resolution == LDPR_PREVAILING_DEF_IRONLY_EXP && !Res.IsLinkonceOdr) if (Resolution == LDPR_PREVAILING_DEF_IRONLY_EXP && !Res.IsLinkonceOdr)
Resolution = LDPR_PREVAILING_DEF; Resolution = LDPR_PREVAILING_DEF;
// In ThinLTO mode change all prevailing resolutions to LDPR_PREVAILING_DEF.
// For ThinLTO the IR files are compiled through the backend independently,
// so we need to ensure that any prevailing linkonce copy will be emitted
// into the object file by making it weak. Additionally, we can skip the
// IRONLY handling for internalization, which isn't performed in ThinLTO
// mode currently anyway.
if (options::thinlto && (Resolution == LDPR_PREVAILING_DEF_IRONLY_EXP ||
Resolution == LDPR_PREVAILING_DEF_IRONLY))
Resolution = LDPR_PREVAILING_DEF;
GV->setUnnamedAddr(Res.UnnamedAddr); GV->setUnnamedAddr(Res.UnnamedAddr);
GV->setVisibility(Res.Visibility); GV->setVisibility(Res.Visibility);
// Override gold's resolution for common symbols. We want the largest // Override gold's resolution for common symbols. We want the largest
// one to win. // one to win.
if (GV->hasCommonLinkage()) { if (GV->hasCommonLinkage()) {
cast<GlobalVariable>(GV)->setAlignment(Res.CommonAlign); cast<GlobalVariable>(GV)->setAlignment(Res.CommonAlign);
if (Resolution == LDPR_PREVAILING_DEF_IRONLY) if (Resolution == LDPR_PREVAILING_DEF_IRONLY)
▲ Show 20 Lines • Show All 563 Lines • Show Last 20 Lines