This is an archive of the discontinued LLVM Phabricator instance.

[InstrProfiling] Keep profd non-private for non-renamable comdat functions
ClosedPublic

Authored by MaskRay on Aug 19 2021, 6:38 PM.

Details

Summary

The NS==0 condition used by D103717 missed a corner case:
if the current copy does not have a hash suffix (e.g. weak_odr), a copy
with value profiling (with a different CFG) may exist. This is super
rare, but is possible with regular PGO (which can make a weak_odr function
inlines its callees differently, sometimes with value profiling while
sometimes without).

If the current copy with private profd is prevailing, the non-prevailing
copy may get an undefined symbol if a caller inlining the non-prevailing
function references its profd. If the other copy with non-private profd
is prevailing, the current copy may cause a "relocation to discarded
section" linker error.

The fix is straightforward: just keep non-private profd in this case.

With this change, a stage 2 (-DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_BUILD_INSTRUMENTED=IR)
clang is 0.08% larger (172431496/172286720-1).
`stat -c %s **/*.o | awk '{s+=$1}END{print s}' is 0.026% larger.
The majority of D103717's benefits remains.

Diff Detail

Unit TestsFailed

Event Timeline

MaskRay created this revision.Aug 19 2021, 6:38 PM
MaskRay requested review of this revision.Aug 19 2021, 6:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2021, 6:38 PM
MaskRay added inline comments.
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
965–971

canRenameComdatFunc should check whether the variable is actually renamed.

MaskRay updated this revision to Diff 367717.Aug 19 2021, 10:55 PM
MaskRay retitled this revision from [InstrProfiling] Don't make non-renamable {weak,linkonce}{,_odr} profd private to [InstrProfiling] Keep profd non-private for non-renamable comdat functions.
MaskRay edited the summary of this revision. (Show Details)

.

MaskRay edited the summary of this revision. (Show Details)

"This is super rare, but is possible with CSPGO (which can make a weak_odr function inlines its callees differently, sometimes with value profiling while sometimes without)."

This may not be an issue to CSPGO, if the IRPGOFlag can be correctly handled as addressed at https://reviews.llvm.org/D107034.
So this may only happen with -fprofile-instr-generate, which should be in a rare case (not sure if it's possible), but not specific to CSPGO.

This looks reasonable. How much size impact does this patch have?

MaskRay added a comment.EditedAug 20 2021, 3:28 PM

This looks reasonable. How much size impact does this patch have?

With this change, a stage 2 (-DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_BUILD_INSTRUMENTED=IR)
clang is 0.08% larger (172431496/172286720-1).
`stat -c %s **/*.o | awk '{s+=$1}END{print s}' is 0.026% larger.
The majority of D103717's benefits remains.

The additional __profd_ symbols are due to weak_odr functions, which are mostly explicit template instantiations.

"This is super rare, but is possible with CSPGO (which can make a weak_odr function inlines its callees differently, sometimes with value profiling while sometimes without)."

This may not be an issue to CSPGO, if the IRPGOFlag can be correctly handled as addressed at https://reviews.llvm.org/D107034.
So this may only happen with -fprofile-instr-generate, which should be in a rare case (not sure if it's possible), but not specific to CSPGO.

Replied there. I think isIRPGOFlagSet(M) => enablesValueProfiling(*M) is not correct.

MaskRay edited the summary of this revision. (Show Details)Aug 20 2021, 3:30 PM
xur added a comment.Aug 23 2021, 11:20 PM

The problem you described is exactly what we wanted to solve with comdat renaming.
We had some correctness issues so I put many restrictions here. We have switched the default linker to lld
and linkage handling has evolved quite bit. I might revisit some of
the restrictions again, like if we can enable the renaming for weak linkage.

The problem can happen for regular PGO. As for CSPGO, the instrumentation is post inline
so the prevailing selection happens before instrumentation. The non-prevailing copy is already
dropped. I don't this will happen in CSPGO.

Anyway, being conservative is reasonable here. This looks good to me.

MaskRay edited the summary of this revision. (Show Details)Aug 23 2021, 11:31 PM
xur accepted this revision.Aug 24 2021, 9:43 AM
This revision is now accepted and ready to land.Aug 24 2021, 9:43 AM
MaskRay edited the summary of this revision. (Show Details)Aug 24 2021, 3:56 PM
This revision was landed with ongoing or failed builds.Aug 24 2021, 3:59 PM
This revision was automatically updated to reflect the committed changes.