This is an archive of the discontinued LLVM Phabricator instance.

[IPO] Opt-in local clones for thinlto imports
ClosedPublic

Authored by mtrofin on May 8 2023, 3:39 PM.

Details

Summary

ThinLTO imports (which appear as available_externally) that survive
inlining get deleted. With today's inliner that's reasonable, because
the way the function would be inlined into in other modules would be the
same - because of the bottom-up traversal assumption, and the fact that
the inliner doesn't take into account surrounding context [*]. The
ModuleInliner invalidates the first assumption, and the ML inliner the
second.

This patch adds a way to opt-in a module to keep its variant of an
imported function, even if it survived past inlining.

  • Almost. Deferred inlining is an exception which can lead to

(empirically) infrequent discrepancies.

Diff Detail

Event Timeline

mtrofin created this revision.May 8 2023, 3:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2023, 3:39 PM
mtrofin requested review of this revision.May 8 2023, 3:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2023, 3:39 PM
snehasish added inline comments.
llvm/lib/Transforms/IPO/ElimAvailExtern.cpp
60

Should we teach sample prof [1] about this new suffix too? On the other hand from the comment it seems like reusing the "__uniq" suffix might be another option?

[1] https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ProfileData/SampleProf.h#L1079-L1083

mtrofin added inline comments.May 8 2023, 4:52 PM
llvm/lib/Transforms/IPO/ElimAvailExtern.cpp
60

The name could have __uniq already; hmm... if we use __uniq here, too, it seems like that's all that'd be needed for teaching sample prof, right?

snehasish added inline comments.May 8 2023, 5:08 PM
llvm/lib/Transforms/IPO/ElimAvailExtern.cpp
60

Yeah, that's my understanding.

mtrofin updated this revision to Diff 520703.May 9 2023, 8:06 AM

use __uniq

mtrofin marked 2 inline comments as done.May 9 2023, 8:06 AM

Can we do this during the importing process? E.g. somewhere in FunctionImportGlobalProcessing::processGlobalForThinLTO.

Can we do this during the importing process? E.g. somewhere in FunctionImportGlobalProcessing::processGlobalForThinLTO.

(capturing discussion offline) I actually had it that way originally, but there are 2 main complications with it. The most important one is about ICP. If we did this before any ICP for a function F, we'd have to remember the association between F and F.__uniq.123 (the latter is the local copy), so that the ICP test would be in terms of F, not F.__uniq.123. With how the pass is currently positioned, that's not an issue, because ICP happened, and the non-CallBase use of F (the value test, in this case) isn't modified. A good way to keep that association (and make it explicit and intentional) is via metadata, but it'd complicate the implementation.

The second (and related) problem is that we'd also need to update the value profile to reference the new GUID instead of the old one (as GUIDs are computed using both name and linkage), which the current implementation doesn't need to do (and I need to add a comment about that.)

We can revisit this later, of course.

mtrofin updated this revision to Diff 521415.May 11 2023, 12:47 PM

more comments

Just a few minor comments/suggestions

llvm/lib/Transforms/IPO/ElimAvailExtern.cpp
49

In theory we could have a non-thinlto available_externally function, so maybe rename this to something more generic like convertToCopy.

61

Maybe just pass this in from the caller that already has it.

70

Nit: remove braces

73

Incomplete comment?

104

Nit remove braces around single statement if / else bodies

112

This tracks the removed functions (per string in STATISTIC). Maybe add a second one for tracking the number of conversions? If you do so, add -stats to your new test and check it.

mtrofin updated this revision to Diff 521481.May 11 2023, 3:38 PM
mtrofin marked 5 inline comments as done.

stats

tejohnson accepted this revision.May 11 2023, 3:55 PM

lgtm

llvm/test/Transforms/EliminateAvailableExternally/transform-to-local.ll
2

I think you will need to add a "REQUIRES: asserts" to avoid bot failures as stats requires that.

This revision is now accepted and ready to land.May 11 2023, 3:55 PM
This revision was landed with ongoing or failed builds.May 11 2023, 3:57 PM
This revision was automatically updated to reflect the committed changes.