Skip to content

Commit d97dae9

Browse files
authored
Revert part of #7715: Ignore metadata in function comparisons (#7737)
Our comparisons are only used in optimization atm, and we decided not to consider metadata when merging functions, as that is what LLVM does: it will fold code together without carefully checking if profiling info matches, and without even trying to fix up that profiling info later. For more on LLVM, see discussion in #7733, but basically LLVM will just merge two instructions with different branch_weights and keep the first of the two, no matter what.
1 parent ca59e79 commit d97dae9

File tree

2 files changed

+17
-40
lines changed

2 files changed

+17
-40
lines changed

src/ir/function-utils.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
#ifndef wasm_ir_function_h
1818
#define wasm_ir_function_h
1919

20-
#include "ir/metadata.h"
2120
#include "ir/utils.h"
2221
#include "wasm.h"
2322

@@ -38,9 +37,10 @@ inline bool equal(Function* left, Function* right) {
3837
return false;
3938
}
4039
}
41-
if (!metadata::equal(left, right)) {
42-
return false;
43-
}
40+
// We could in principle compare metadata here, but intentionally do not, as
41+
// for optimization purposes we do want to e.g. merge functions that differ
42+
// only in metadata (following LLVM's example). If we have a non-optimization
43+
// reason for comparing metadata here then we could add a flag for it.
4444
if (left->imported() && right->imported()) {
4545
return true;
4646
}

test/lit/passes/duplicate-function-elimination_branch-hints.wast

Lines changed: 13 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,16 @@
33

44
;; RUN: foreach %s %t wasm-opt --duplicate-function-elimination --all-features -S -o - | filecheck %s
55

6-
;; The functions here differ in branch hints, and should not be merged.
6+
;; Test that we merge functions even if they differ in branch hints. This is
7+
;; good for code size, and follows what LLVM does.
8+
9+
;; The functions here differ in branch hints (but we still merge).
710
(module
811
;; CHECK: (type $0 (func (param i32)))
912

1013
;; CHECK: (export "a" (func $a))
1114

12-
;; CHECK: (export "b" (func $b))
15+
;; CHECK: (export "b" (func $a))
1316

1417
;; CHECK: (func $a (type $0) (param $x i32)
1518
;; CHECK-NEXT: (@metadata.code.branch_hint "\00")
@@ -30,15 +33,6 @@
3033
)
3134
)
3235

33-
;; CHECK: (func $b (type $0) (param $x i32)
34-
;; CHECK-NEXT: (@metadata.code.branch_hint "\01")
35-
;; CHECK-NEXT: (if
36-
;; CHECK-NEXT: (local.get $x)
37-
;; CHECK-NEXT: (then
38-
;; CHECK-NEXT: (unreachable)
39-
;; CHECK-NEXT: )
40-
;; CHECK-NEXT: )
41-
;; CHECK-NEXT: )
4236
(func $b (export "b") (param $x i32)
4337
(@metadata.code.branch_hint "\01")
4438
(if
@@ -50,14 +44,13 @@
5044
)
5145
)
5246

53-
;; These also differ, now one is missing a hint, and they should not be merged.
54-
;; TODO: Perhaps when optimizing for size, we should merge and drop the hint?
47+
;; These also differ, now one is missing a hint (but we still merge).
5548
(module
5649
;; CHECK: (type $0 (func (param i32)))
5750

5851
;; CHECK: (export "a" (func $a))
5952

60-
;; CHECK: (export "b" (func $b))
53+
;; CHECK: (export "b" (func $a))
6154

6255
;; CHECK: (func $a (type $0) (param $x i32)
6356
;; CHECK-NEXT: (@metadata.code.branch_hint "\00")
@@ -78,14 +71,6 @@
7871
)
7972
)
8073

81-
;; CHECK: (func $b (type $0) (param $x i32)
82-
;; CHECK-NEXT: (if
83-
;; CHECK-NEXT: (local.get $x)
84-
;; CHECK-NEXT: (then
85-
;; CHECK-NEXT: (unreachable)
86-
;; CHECK-NEXT: )
87-
;; CHECK-NEXT: )
88-
;; CHECK-NEXT: )
8974
(func $b (export "b") (param $x i32)
9075
(if
9176
(local.get $x)
@@ -97,13 +82,13 @@
9782
)
9883

9984
;; Flipped case of the above, now the other one is the only one with a hint,
100-
;; and that hint is flipped.
85+
;; and that hint is flipped (but we still merge).
10186
(module
10287
;; CHECK: (type $0 (func (param i32)))
10388

10489
;; CHECK: (export "a" (func $a))
10590

106-
;; CHECK: (export "b" (func $b))
91+
;; CHECK: (export "b" (func $a))
10792

10893
;; CHECK: (func $a (type $0) (param $x i32)
10994
;; CHECK-NEXT: (if
@@ -122,15 +107,6 @@
122107
)
123108
)
124109

125-
;; CHECK: (func $b (type $0) (param $x i32)
126-
;; CHECK-NEXT: (@metadata.code.branch_hint "\01")
127-
;; CHECK-NEXT: (if
128-
;; CHECK-NEXT: (local.get $x)
129-
;; CHECK-NEXT: (then
130-
;; CHECK-NEXT: (unreachable)
131-
;; CHECK-NEXT: )
132-
;; CHECK-NEXT: )
133-
;; CHECK-NEXT: )
134110
(func $b (export "b") (param $x i32)
135111
(@metadata.code.branch_hint "\01")
136112
(if
@@ -142,7 +118,7 @@
142118
)
143119
)
144120

145-
;; Identical branch hints: We can merge here.
121+
;; Identical branch hints: We can definitely merge here.
146122
(module
147123
;; CHECK: (type $0 (func (param i32)))
148124

@@ -218,8 +194,9 @@
218194
)
219195
)
220196

221-
;; Source file location (debug info) does *not* prevent optimization. We
222-
;; prioritize optimization over debug info quality.
197+
;; Source file location (debug info) does not prevent optimization (and has
198+
;; even less reason to do so than branch hints, as we prioritize optimization
199+
;; over debug info quality).
223200
(module
224201
;; CHECK: (type $0 (func (param i32)))
225202

0 commit comments

Comments
 (0)