Skip to content

Commit 8c82b68

Browse files
kripkenkripken
authored
[Branch Hints][DWARF] Fix hints on stacky code, by making IRBuilder aware of synthetic code (#7640)
Synthetic code, like for scratch locals, has no binary location. Before, we thought it did, which messed up location tracking for surrounding code.
1 parent 700fa15 commit 8c82b68

File tree

5 files changed

+107
-26
lines changed

5 files changed

+107
-26
lines changed

‎src/passes/Outlining.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ struct ReconstructStringifyWalker
128128
// visitIfStart(), which will expect to be able to pop the condition.
129129
// This is always okay to do because the correct condition was installed
130130
// onto the If when the outer scope was visited.
131-
existingBuilder.push(curr->iff->condition);
131+
existingBuilder.pushSynthetic(curr->iff->condition);
132132
ODBG(desc = "If Start at ");
133133
ASSERT_OK(existingBuilder.visitIfStart(curr->iff));
134134
} else if (reason.getElseStart()) {

‎src/wasm-ir-builder.h

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,21 @@ class IRBuilder : public UnifiedExpressionVisitor<IRBuilder, Result<>> {
5555
// initialized to initialize the child fields and refinalize it.
5656
Result<> visit(Expression*);
5757

58+
// The origin of an expression.
59+
enum class Origin {
60+
// The expression originates in the binary we are reading. We track binary
61+
// locations for such instructions where necessary (for code annotations,
62+
// etc.).
63+
Binary = 0,
64+
// The expression was synthetically added by the IRBuilder (e.g. a local.get
65+
// of a scratch local).
66+
Synthetic = 1
67+
};
68+
5869
// Like visit, but pushes the expression onto the stack as-is without popping
5970
// any children or refinalization.
60-
void push(Expression*);
71+
void push(Expression*, Origin origin = Origin::Binary);
72+
void pushSynthetic(Expression* expr) { push(expr, Origin::Synthetic); }
6173

6274
// Set the debug location to be attached to the next visited, created, or
6375
// pushed instruction.

‎src/wasm/wasm-ir-builder.cpp

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ MaybeResult<IRBuilder::HoistedVal> IRBuilder::hoistLastValue() {
7979
if (type == Type::unreachable) {
8080
// Make sure the top of the stack also has an unreachable expression.
8181
if (stack.back()->type != Type::unreachable) {
82-
push(builder.makeUnreachable());
82+
pushSynthetic(builder.makeUnreachable());
8383
}
8484
return HoistedVal{Index(index), nullptr};
8585
}
@@ -88,7 +88,7 @@ MaybeResult<IRBuilder::HoistedVal> IRBuilder::hoistLastValue() {
8888
CHECK_ERR(scratchIdx);
8989
expr = builder.makeLocalSet(*scratchIdx, expr);
9090
auto* get = builder.makeLocalGet(*scratchIdx, type);
91-
push(get);
91+
pushSynthetic(get);
9292
return HoistedVal{Index(index), get};
9393
}
9494

@@ -107,7 +107,7 @@ Result<> IRBuilder::packageHoistedValue(const HoistedVal& hoisted,
107107
scope.exprStack.end());
108108
auto* block = builder.makeBlock(exprs, type);
109109
scope.exprStack.resize(hoisted.valIndex);
110-
push(block);
110+
pushSynthetic(block);
111111
};
112112

113113
auto type = scope.exprStack.back()->type;
@@ -137,33 +137,37 @@ Result<> IRBuilder::packageHoistedValue(const HoistedVal& hoisted,
137137
scratchIdx = *scratch;
138138
}
139139
for (Index i = 1, size = type.size(); i < size; ++i) {
140-
push(builder.makeTupleExtract(builder.makeLocalGet(scratchIdx, type), i));
140+
pushSynthetic(
141+
builder.makeTupleExtract(builder.makeLocalGet(scratchIdx, type), i));
141142
}
142143
return Ok{};
143144
}
144145

145-
void IRBuilder::push(Expression* expr) {
146+
void IRBuilder::push(Expression* expr, Origin origin) {
146147
auto& scope = getScope();
147148
if (expr->type == Type::unreachable) {
148149
scope.unreachable = true;
149150
}
150151
scope.exprStack.push_back(expr);
151152

152-
applyDebugLoc(expr);
153-
if (binaryPos && func && lastBinaryPos != *binaryPos) {
154-
auto span =
155-
BinaryLocations::Span{BinaryLocation(lastBinaryPos - codeSectionOffset),
156-
BinaryLocation(*binaryPos - codeSectionOffset)};
157-
// Some expressions already have their start noted, and we are just seeing
158-
// their last segment (like an Else).
159-
auto [iter, inserted] = func->expressionLocations.insert({expr, span});
160-
if (!inserted) {
161-
// Just update the end.
162-
iter->second.end = span.end;
163-
// The true start from before is before the start of the current segment.
164-
assert(iter->second.start < span.start);
153+
if (origin == Origin::Binary) {
154+
applyDebugLoc(expr);
155+
if (binaryPos && func && lastBinaryPos != *binaryPos) {
156+
auto span =
157+
BinaryLocations::Span{BinaryLocation(lastBinaryPos - codeSectionOffset),
158+
BinaryLocation(*binaryPos - codeSectionOffset)};
159+
// Some expressions already have their start noted, and we are just seeing
160+
// their last segment (like an Else).
161+
auto [iter, inserted] = func->expressionLocations.insert({expr, span});
162+
if (!inserted) {
163+
// Just update the end.
164+
iter->second.end = span.end;
165+
// The true start from before is before the start of the current
166+
// segment.
167+
assert(iter->second.start < span.start);
168+
}
169+
lastBinaryPos = *binaryPos;
165170
}
166-
lastBinaryPos = *binaryPos;
167171
}
168172

169173
DBG(std::cerr << "After pushing " << ShallowExpression{expr} << ":\n");
@@ -1018,7 +1022,7 @@ Result<> IRBuilder::visitCatch(Name tag) {
10181022
// Note that we have a pop to help determine later whether we need to run
10191023
// the fixup for pops within blocks.
10201024
scopeStack[0].notePop();
1021-
push(builder.makePop(params));
1025+
pushSynthetic(builder.makePop(params));
10221026
}
10231027

10241028
return Ok{};

‎test/lit/branch-hinting-stack-ir.wast

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
2+
3+
;; Enable StackIR opts with --shrink-level=1, and verify we can roundtrip an
4+
;; annotation on stacky code.
5+
6+
;; RUN: wasm-opt -all --shrink-level=1 --roundtrip %s -S -o - | filecheck %s
7+
8+
(module
9+
;; CHECK: (type $0 (func))
10+
11+
;; CHECK: (func $empty-if (type $0)
12+
;; CHECK-NEXT: (local $1 i32)
13+
;; CHECK-NEXT: (local $scratch i32)
14+
;; CHECK-NEXT: (@metadata.code.branch_hint "\00")
15+
;; CHECK-NEXT: (if
16+
;; CHECK-NEXT: (block (result i32)
17+
;; CHECK-NEXT: (local.set $scratch
18+
;; CHECK-NEXT: (i32.const 0)
19+
;; CHECK-NEXT: )
20+
;; CHECK-NEXT: (drop
21+
;; CHECK-NEXT: (i32.const 1)
22+
;; CHECK-NEXT: )
23+
;; CHECK-NEXT: (local.get $scratch)
24+
;; CHECK-NEXT: )
25+
;; CHECK-NEXT: (then
26+
;; CHECK-NEXT: )
27+
;; CHECK-NEXT: (else
28+
;; CHECK-NEXT: )
29+
;; CHECK-NEXT: )
30+
;; CHECK-NEXT: )
31+
(func $empty-if
32+
(local $1 i32)
33+
;; Several stack IR opts work here, leading to the if arms being empty (nops
34+
;; removed) and the local.set/get vanishing, leaving stacky code like this:
35+
;;
36+
;; i32.const 0 ;; read by the if, past the other const and drop
37+
;; i32.const 1
38+
;; drop
39+
;; if
40+
;; else
41+
;; end
42+
;;
43+
;; As a result we have a segment before us that was heavily modified (with the
44+
;; local.set), and the if body is empty. This should not cause an error when
45+
;; computing the if's binary location for the hint, and the hint should
46+
;; remain.
47+
(local.set $1
48+
(i32.const 0)
49+
)
50+
(drop
51+
(i32.const 1)
52+
)
53+
(@metadata.code.branch_hint "\00")
54+
(if
55+
(local.get $1)
56+
(then
57+
(nop)
58+
)
59+
(else
60+
(nop)
61+
)
62+
)
63+
)
64+
)
65+

‎test/passes/fannkuch3_manyopts_dwarf.bin.txt

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4902,6 +4902,7 @@ file_names[ 4]:
49024902
;; code offset: 0xaf
49034903
(local.get $2)
49044904
)
4905+
;; code offset: 0xbd
49054906
(br_if $label1
49064907
(block (result i32)
49074908
(local.set $scratch
@@ -4918,7 +4919,6 @@ file_names[ 4]:
49184919
;; code offset: 0xb9
49194920
(local.get $1)
49204921
)
4921-
;; code offset: 0xbd
49224922
(local.get $scratch)
49234923
)
49244924
)
@@ -5428,6 +5428,7 @@ file_names[ 4]:
54285428
;; code offset: 0x234
54295429
(local.get $2)
54305430
)
5431+
;; code offset: 0x242
54315432
(br_if $label7
54325433
(block (result i32)
54335434
(local.set $scratch_18
@@ -5444,7 +5445,6 @@ file_names[ 4]:
54445445
;; code offset: 0x23e
54455446
(local.get $1)
54465447
)
5447-
;; code offset: 0x242
54485448
(local.get $scratch_18)
54495449
)
54505450
)
@@ -6252,6 +6252,7 @@ file_names[ 4]:
62526252
;; code offset: 0x4a7
62536253
(local.get $2)
62546254
)
6255+
;; code offset: 0x4b5
62556256
(br_if $label3
62566257
(block (result i32)
62576258
(local.set $scratch
@@ -6268,7 +6269,6 @@ file_names[ 4]:
62686269
;; code offset: 0x4b1
62696270
(local.get $0)
62706271
)
6271-
;; code offset: 0x4b5
62726272
(local.get $scratch)
62736273
)
62746274
)
@@ -6515,6 +6515,7 @@ file_names[ 4]:
65156515
;; code offset: 0x565
65166516
(local.get $2)
65176517
)
6518+
;; code offset: 0x573
65186519
(br_if $label7
65196520
(block (result i32)
65206521
(local.set $scratch_10
@@ -6531,7 +6532,6 @@ file_names[ 4]:
65316532
;; code offset: 0x56f
65326533
(local.get $0)
65336534
)
6534-
;; code offset: 0x573
65356535
(local.get $scratch_10)
65366536
)
65376537
)

0 commit comments

Comments
 (0)