llvm.org GIT mirror llvm / 19a9621
[MergeFuncs] Improve ordering of equal functions 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. Reviewers: jfb, whitequark Reviewed By: whitequark Subscribers: rkruppe, llvm-commits Differential Revision: https://reviews.llvm.org/D53271 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@346386 91177308-0d34-0410-b5e6-96231b3b80d8 whitequark 10 months ago
2 changed file(s) with 76 addition(s) and 9 deletion(s). Raw diff Collapse all Expand all
283283 // modified, i.e. in insert(), remove(), and replaceFunctionInTree(), to avoid
284284 // dangling iterators into FnTree. The invariant that preserves this is that
285285 // there is exactly one mapping F -> FN for each FunctionNode FN in FnTree.
286 ValueMap, FnTreeType::iterator> FNodesInTree;
286 DenseMap, FnTreeType::iterator> FNodesInTree;
287287 };
288288
289289 } // end anonymous namespace
424424 } while (!Deferred.empty());
425425
426426 FnTree.clear();
427 FNodesInTree.clear();
427428 GlobalNumbers.clear();
428429
429430 return Changed;
816817 FN.replaceBy(G);
817818 }
818819
820 // Ordering for functions that are equal under FunctionComparator
821 static bool isFuncOrderCorrect(const Function *F, const Function *G) {
822 if (F->isInterposable() != G->isInterposable()) {
823 // Strong before weak, because the weak function may call the strong
824 // one, but not the other way around.
825 return !F->isInterposable();
826 }
827 if (F->hasLocalLinkage() != G->hasLocalLinkage()) {
828 // External before local, because we definitely have to keep the external
829 // function, but may be able to drop the local one.
830 return !F->hasLocalLinkage();
831 }
832 // Impose a total order (by name) on the replacement of functions. This is
833 // important when operating on more than one module independently to prevent
834 // cycles of thunks calling each other when the modules are linked together.
835 return F->getName() <= G->getName();
836 }
837
819838 // Insert a ComparableFunction into the FnTree, or merge it away if equal to one
820839 // that was already inserted.
821840 bool MergeFunctions::insert(Function *NewFunction) {
832851
833852 const FunctionNode &OldF = *Result.first;
834853
835 // Impose a total order (by name) on the replacement of functions. This is
836 // important when operating on more than one module independently to prevent
837 // cycles of thunks calling each other when the modules are linked together.
838 //
839 // First of all, we process strong functions before weak functions.
840 if ((OldF.getFunc()->isInterposable() && !NewFunction->isInterposable()) ||
841 (OldF.getFunc()->isInterposable() == NewFunction->isInterposable() &&
842 OldF.getFunc()->getName() > NewFunction->getName())) {
854 if (!isFuncOrderCorrect(OldF.getFunc(), NewFunction)) {
843855 // Swap the two functions.
844856 Function *F = OldF.getFunc();
845857 replaceFunctionInTree(*Result.first, NewFunction);
0 ; RUN: opt -S -mergefunc < %s | FileCheck %s
1
2 ; We should normalize to test2 rather than test1,
3 ; because it allows us to drop test1 entirely
4
5 ; CHECK-NOT: define internal void @test1() unnamed_addr
6 ; CHECK: define void @test3() unnamed_addr
7 ; CHECK-NEXT: call void @test2()
8 ; CHECK-NEXT: call void @test2()
9
10 declare void @dummy()
11
12 define internal void @test1() unnamed_addr {
13 call void @dummy()
14 call void @dummy()
15 ret void
16 }
17
18 define void @test2() unnamed_addr {
19 call void @dummy()
20 call void @dummy()
21 ret void
22 }
23
24 define void @test3() unnamed_addr {
25 call void @test1()
26 call void @test2()
27 ret void
28 }
29
30 ; We should normalize to the existing test6 rather than
31 ; to a new anonymous strong backing function
32
33 ; CHECK: define weak void @test5()
34 ; CHECK-NEXT: tail call void @test6()
35 ; CHECK: define weak void @test4()
36 ; CHECK-NEXT: tail call void @test6()
37
38 declare void @dummy2()
39
40 define weak void @test4() {
41 call void @dummy2()
42 call void @dummy2()
43 ret void
44 }
45 define weak void @test5() {
46 call void @dummy2()
47 call void @dummy2()
48 ret void
49 }
50 define void @test6() {
51 call void @dummy2()
52 call void @dummy2()
53 ret void
54 }