Page MenuHomePhabricator

[MergeFuncs] Generate alias instead of thunk if possible (default disabled)
ClosedPublic

Authored by nikic on Oct 15 2018, 5:17 AM.

Details

Summary

The MergeFunctions pass was originally intended to emit aliases
instead of thunks where possible (unnamed_addr). However, for a
long time this functionality was behind a flag hardcoded to false,
bitrotted and was eventually removed in
https://github.com/llvm-mirror/llvm/commit/f9f40b539b1dc44b10184684aa3f72e1192c454f.

Originally the functionality was first disabled in
https://github.com/llvm-mirror/llvm/commit/664040a03c17f432a127a35013eeb6b6a26e41fb
due to lack of support for aliases in Mach-O. I believe that this
is no longer the case nowadays, but not really familiar with this
area.

In the interest of being conservative, this patch reintroduces the
aliasing functionality (with some fixes) behind a default disabled
-mergefunc-use-aliases flag.

Diff Detail

Repository
rL LLVM

Event Timeline

nikic created this revision.Oct 15 2018, 5:17 AM
whitequark added inline comments.Oct 28 2018, 11:55 PM
lib/Transforms/IPO/MergeFunctions.cpp
803 ↗(On Diff #169682)

Can you explain this logic a bit? It's not clear to me why the condition should be written like that (two canCreateAliasFor` calls).

nikic added inline comments.Oct 29 2018, 2:23 AM
lib/Transforms/IPO/MergeFunctions.cpp
803 ↗(On Diff #169682)

The idea here is that we need both of the writeThunkOrAlias calls below to succeed, otherwise we might be leaving behind functions in a partially rewired state. Both calls will succeed if either both G and H below can be converted to aliases (are global unnamed addr) or if a thunk for F below is profitable.

The renaming that is going on here makes it slightly confusing, in that H below corresponds to F here for the purpose of the (signature-based) alias check and F below corresponds to F here for the purpose of the (body-based) thunk-profitability check.

whitequark added inline comments.Oct 29 2018, 2:57 AM
lib/Transforms/IPO/MergeFunctions.cpp
803 ↗(On Diff #169682)

Right. Do you think you can add this as a comment?

nikic updated this revision to Diff 171470.Oct 29 2018, 3:18 AM

Added comment about thunk/alias checks for the weak-weak case. Also renamed the function H to NewF, because I think this makes it slightly more obvious what is going on in that case.

whitequark accepted this revision.Oct 29 2018, 3:19 AM
This revision is now accepted and ready to land.Oct 29 2018, 3:19 AM
nikic added a comment.Oct 30 2018, 3:24 PM

As I don't have commit rights, could you please commit D53262, D53271 and this revision?

rkruppe added a subscriber: rkruppe.Nov 5 2018, 5:35 AM

As I don't have commit rights, could you please commit D53262, D53271 and this revision?

Committed D53262 and D53271 but this one doesn't seem to apply.

nikic updated this revision to Diff 173233.Nov 8 2018, 2:56 PM

Rebase patch to current master.

This revision was automatically updated to reflect the committed changes.