Page MenuHomePhabricator

[MergeFuncs] Improve ordering of equal functions
ClosedPublic

Authored by nikic on Oct 15 2018, 1:49 AM.

Details

Summary

MergeFunctions currently tries to process strong functions before
weak functions, because weak functions can simply call strong
functions, while a strong/weak function cannot call a weak function
(a backing strong function is needed).

This patch additionally tries to process external functions before
local functions, because we definitely have to keep the external
function, but may be able to drop the local one (and definitely
can if it is also unnamed_addr).

Unfortunately, this exposes an existing bug in the implementation:
The FnTree and FNodesInTree structures can currently go out of
sync in the case where two weak functions are merged, because the
function in FnTree/FNodesInTree is RAUWed. This leaves it behind in
FnTree (this is intended, as it is the strong backing function which
should be used for further merges), while it is replaced in
FNodesInTree (this is not intended).

This is fixed by switching FNodesInTree from using a ValueMap to
using a DenseMap of AssertingVH.

This exposes another minor issue: Currently FNodesInTree is not
cleared after MergeFunctions finishes running. Currently, this is
potentially dangerous (e.g. if something else wants to RAUW a function
with a non-function), but at the very least it is unnecessary/inefficient.
After the change to use AssertingVH it becomes more problematic,
because there are certainly passes that remove functions.

This issue is fixed by clearing FNodesInTree at the end of the pass.

Diff Detail

Repository
rL LLVM

Event Timeline

nikic created this revision.Oct 15 2018, 1:49 AM
whitequark accepted this revision.Oct 28 2018, 11:52 PM
This revision is now accepted and ready to land.Oct 28 2018, 11:52 PM
rkruppe added a subscriber: rkruppe.Nov 5 2018, 5:34 AM
This revision was automatically updated to reflect the committed changes.