Skip to content

Commit 46054db

Browse files
dcodeIOdcodeIO
authored
Fix loop RC issues (#918)
1 parent a4d5f09 commit 46054db

30 files changed

+1077
-638
lines changed

‎src/compiler.ts‎

Lines changed: 44 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1909,12 +1909,25 @@ export class Compiler extends DiagnosticEmitter {
19091909
var outerFlow = this.currentFlow;
19101910
var label = outerFlow.pushBreakLabel();
19111911
var innerFlow = outerFlow.fork();
1912-
this.currentFlow = innerFlow;
19131912
var breakLabel = "break|" + label;
19141913
innerFlow.breakLabel = breakLabel;
19151914
var continueLabel = "continue|" + label;
19161915
innerFlow.continueLabel = continueLabel;
19171916

1917+
// Compile the condition before the body in order to...
1918+
var condFlow = outerFlow.fork();
1919+
this.currentFlow = condFlow;
1920+
var condExpr = module.precomputeExpression(
1921+
this.makeIsTrueish(
1922+
this.compileExpression(statement.condition, Type.i32),
1923+
this.currentType
1924+
)
1925+
);
1926+
assert(!condFlow.hasScopedLocals);
1927+
// ...unify local states before and after the condition has been executed the first time
1928+
innerFlow.unifyLocalFlags(condFlow);
1929+
this.currentFlow = innerFlow;
1930+
19181931
var stmts = new Array<ExpressionRef>();
19191932
if (statement.statement.kind == NodeKind.BLOCK) {
19201933
this.compileStatements((<BlockStatement>statement.statement).statements, false, stmts);
@@ -1923,12 +1936,6 @@ export class Compiler extends DiagnosticEmitter {
19231936
this.compileStatement(statement.statement)
19241937
);
19251938
}
1926-
var condExpr = module.precomputeExpression(
1927-
this.makeIsTrueish(
1928-
this.compileExpression(statement.condition, Type.i32),
1929-
this.currentType
1930-
)
1931-
);
19321939
var alwaysFalse = false;
19331940
if (getExpressionId(condExpr) == ExpressionId.Const) {
19341941
assert(getExpressionType(condExpr) == NativeType.I32);
@@ -1946,8 +1953,11 @@ export class Compiler extends DiagnosticEmitter {
19461953
// )
19471954
var fallsThrough = !terminates && !innerFlow.is(FlowFlags.BREAKS);
19481955

1949-
if (fallsThrough && !alwaysFalse) { // (4)
1950-
stmts.push(module.br(continueLabel, condExpr));
1956+
if (fallsThrough) {
1957+
this.performAutoreleases(innerFlow, stmts);
1958+
if (!alwaysFalse) { // (4)
1959+
stmts.push(module.br(continueLabel, condExpr));
1960+
}
19511961
}
19521962
var expr = flatten(module, stmts, NativeType.None);
19531963
if (fallsThrough && !alwaysFalse || continues) { // (2)
@@ -1958,7 +1968,6 @@ export class Compiler extends DiagnosticEmitter {
19581968
}
19591969

19601970
// Switch back to the parent flow
1961-
if (!terminates) this.performAutoreleases(innerFlow, stmts);
19621971
innerFlow.freeScopedLocals();
19631972
outerFlow.popBreakLabel();
19641973
innerFlow.unset(
@@ -2030,16 +2039,26 @@ export class Compiler extends DiagnosticEmitter {
20302039
}
20312040
innerFlow.inheritNonnullIfTrue(condExpr);
20322041

2033-
// Compile incrementor
2042+
// Compile the incrementor before the body in order to...
20342043
var incrementor = statement.incrementor;
20352044
var incrExpr: ExpressionRef = 0;
2036-
if (incrementor) incrExpr = this.compileExpression(incrementor, Type.void, Constraints.CONV_IMPLICIT | Constraints.WILL_DROP);
2045+
if (incrementor) {
2046+
let incrFlow = innerFlow.fork();
2047+
this.currentFlow = incrFlow;
2048+
incrExpr = this.compileExpression(incrementor, Type.void, Constraints.CONV_IMPLICIT | Constraints.WILL_DROP);
2049+
assert(!incrFlow.hasScopedLocals);
2050+
this.currentFlow = innerFlow;
2051+
// ...unify local states before and after the incrementor has been executed the first time
2052+
innerFlow.unifyLocalFlags(incrFlow);
2053+
}
20372054

20382055
// Compile body (break: drop out, continue: fall through to incrementor, + loop)
2039-
var breakLabel = innerFlow.breakLabel = "break|" + label; innerFlow.breakLabel = breakLabel;
2040-
innerFlow.breakLabel = breakLabel;
2056+
var bodyFlow = innerFlow.fork();
2057+
this.currentFlow = bodyFlow;
2058+
var breakLabel = innerFlow.breakLabel = "break|" + label; bodyFlow.breakLabel = breakLabel;
2059+
bodyFlow.breakLabel = breakLabel;
20412060
var continueLabel = "continue|" + label;
2042-
innerFlow.continueLabel = continueLabel;
2061+
bodyFlow.continueLabel = continueLabel;
20432062
var loopLabel = "loop|" + label;
20442063
var bodyStatement = statement.statement;
20452064
var stmts = new Array<ExpressionRef>();
@@ -2048,9 +2067,16 @@ export class Compiler extends DiagnosticEmitter {
20482067
} else {
20492068
stmts.push(this.compileStatement(bodyStatement));
20502069
}
2051-
var terminates = innerFlow.is(FlowFlags.TERMINATES);
2052-
var continues = innerFlow.isAny(FlowFlags.CONTINUES | FlowFlags.CONDITIONALLY_CONTINUES);
2053-
var breaks = innerFlow.isAny(FlowFlags.BREAKS | FlowFlags.CONDITIONALLY_BREAKS);
2070+
var terminates = bodyFlow.is(FlowFlags.TERMINATES);
2071+
var continues = bodyFlow.isAny(FlowFlags.CONTINUES | FlowFlags.CONDITIONALLY_CONTINUES);
2072+
var breaks = bodyFlow.isAny(FlowFlags.BREAKS | FlowFlags.CONDITIONALLY_BREAKS);
2073+
var fallsThrough = !terminates && !innerFlow.is(FlowFlags.BREAKS);
2074+
2075+
// Finalize body flow
2076+
if (fallsThrough) this.performAutoreleases(bodyFlow, stmts);
2077+
bodyFlow.freeScopedLocals();
2078+
innerFlow.inherit(bodyFlow);
2079+
this.currentFlow = innerFlow;
20542080

20552081
// (block $break ;; (1) skip label (needed anyway) if skipping (4) + no breaks
20562082
// (initializer) ;; (2) [may be empty]
@@ -2063,7 +2089,6 @@ export class Compiler extends DiagnosticEmitter {
20632089
// (br $loop) ;; (8) skip if skipping (3)
20642090
// )
20652091
// )
2066-
var fallsThrough = !terminates && !innerFlow.is(FlowFlags.BREAKS);
20672092
var needsLabel = !alwaysTrue || breaks;
20682093

20692094
var loop = new Array<ExpressionRef>();

‎src/flow.ts‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -440,6 +440,18 @@ export class Flow {
440440
return scopedAlias;
441441
}
442442

443+
/** Tests if this flow has any scoped locals that must be free'd. */
444+
get hasScopedLocals(): bool {
445+
if (this.scopedLocals) {
446+
for (let scopedLocal of this.scopedLocals.values()) {
447+
if (scopedLocal.is(CommonFlags.SCOPED)) { // otherwise an alias
448+
return true;
449+
}
450+
}
451+
}
452+
return false;
453+
}
454+
443455
/** Frees this flow's scoped variables and returns its parent flow. */
444456
freeScopedLocals(): void {
445457
if (this.scopedLocals) {
@@ -592,6 +604,25 @@ export class Flow {
592604
this.localFlags = combinedFlags;
593605
}
594606

607+
/** Unifies local flags between this and the other flow. */
608+
unifyLocalFlags(other: Flow): void {
609+
var numThisLocalFlags = this.localFlags.length;
610+
var numOtherLocalFlags = other.localFlags.length;
611+
for (let i = 0, k = min<i32>(numThisLocalFlags, numOtherLocalFlags); i < k; ++i) {
612+
if (this.isLocalFlag(i, LocalFlags.WRAPPED) != other.isLocalFlag(i, LocalFlags.WRAPPED)) {
613+
this.unsetLocalFlag(i, LocalFlags.WRAPPED); // assume not wrapped
614+
}
615+
if (this.isLocalFlag(i, LocalFlags.NONNULL) != other.isLocalFlag(i, LocalFlags.NONNULL)) {
616+
this.unsetLocalFlag(i, LocalFlags.NONNULL); // assume possibly null
617+
}
618+
assert(
619+
// having different retain states would be a problem because the compiler
620+
// either can't release a retained local or would release a non-retained local
621+
this.isAnyLocalFlag(i, LocalFlags.ANY_RETAINED) == other.isAnyLocalFlag(i, LocalFlags.ANY_RETAINED)
622+
);
623+
}
624+
}
625+
595626
/** Checks if an expression of the specified type is known to be non-null, even if the type might be nullable. */
596627
isNonnull(expr: ExpressionRef, type: Type): bool {
597628
if (!type.is(TypeFlags.NULLABLE)) return true;

‎tests/compiler/loop-wrap.json‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{
2+
"asc_flags": [
3+
"--runtime none"
4+
]
5+
}

‎tests/compiler/loop-wrap.optimized.wat‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
(module
2+
(type $FUNCSIG$v (func))
3+
(type $FUNCSIG$vi (func (param i32)))
4+
(memory $0 0)
5+
(export "memory" (memory $0))
6+
(export "testAlwaysWrapped" (func $loop-wrap/testAlwaysWrapped))
7+
(export "testFirstWrapped" (func $loop-wrap/testFirstWrapped))
8+
(export "testSubsequentWrapped" (func $loop-wrap/testSubsequentWrapped))
9+
(func $loop-wrap/testAlwaysWrapped (; 0 ;) (type $FUNCSIG$v)
10+
(local $0 i32)
11+
loop $continue|0
12+
local.get $0
13+
i32.const 10
14+
i32.ne
15+
if
16+
local.get $0
17+
i32.const 1
18+
i32.add
19+
i32.const 255
20+
i32.and
21+
local.tee $0
22+
br_if $continue|0
23+
end
24+
end
25+
)
26+
(func $loop-wrap/testFirstWrapped (; 1 ;) (type $FUNCSIG$v)
27+
(local $0 i32)
28+
loop $continue|0
29+
local.get $0
30+
i32.const 255
31+
i32.and
32+
i32.const 10
33+
i32.ne
34+
if
35+
local.get $0
36+
i32.const 1
37+
i32.add
38+
local.tee $0
39+
i32.const 255
40+
i32.and
41+
br_if $continue|0
42+
end
43+
end
44+
)
45+
(func $loop-wrap/testSubsequentWrapped (; 2 ;) (type $FUNCSIG$vi) (param $0 i32)
46+
loop $continue|0
47+
local.get $0
48+
i32.const 255
49+
i32.and
50+
i32.const 10
51+
i32.ne
52+
if
53+
local.get $0
54+
i32.const 1
55+
i32.add
56+
i32.const 255
57+
i32.and
58+
local.tee $0
59+
br_if $continue|0
60+
end
61+
end
62+
)
63+
(func $null (; 3 ;) (type $FUNCSIG$v)
64+
nop
65+
)
66+
)

‎tests/compiler/loop-wrap.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
export function testAlwaysWrapped(): void {
2+
var i: u8 = 0; // <--
3+
do {
4+
if (i == 10) break; // no need to wrap
5+
} while (i = (i + 1) & 0xff); // <--
6+
}
7+
8+
export function testFirstWrapped(): void {
9+
var i: u8 = 0;
10+
do {
11+
if (i == 10) break; // must wrap
12+
} while (++i); // <--
13+
}
14+
15+
export function testSubsequentWrapped(a: i32): void {
16+
var i = <u8>a; // <--
17+
do {
18+
if (i == 10) break; // must wrap
19+
} while (i = (i + 1) & 0xff);
20+
}

‎tests/compiler/loop-wrap.untouched.wat‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
(module
2+
(type $FUNCSIG$v (func))
3+
(type $FUNCSIG$vi (func (param i32)))
4+
(memory $0 0)
5+
(table $0 1 funcref)
6+
(elem (i32.const 0) $null)
7+
(export "memory" (memory $0))
8+
(export "testAlwaysWrapped" (func $loop-wrap/testAlwaysWrapped))
9+
(export "testFirstWrapped" (func $loop-wrap/testFirstWrapped))
10+
(export "testSubsequentWrapped" (func $loop-wrap/testSubsequentWrapped))
11+
(func $loop-wrap/testAlwaysWrapped (; 0 ;) (type $FUNCSIG$v)
12+
(local $0 i32)
13+
i32.const 0
14+
local.set $0
15+
block $break|0
16+
loop $continue|0
17+
local.get $0
18+
i32.const 10
19+
i32.eq
20+
if
21+
br $break|0
22+
end
23+
local.get $0
24+
i32.const 1
25+
i32.add
26+
i32.const 255
27+
i32.and
28+
local.tee $0
29+
br_if $continue|0
30+
end
31+
end
32+
)
33+
(func $loop-wrap/testFirstWrapped (; 1 ;) (type $FUNCSIG$v)
34+
(local $0 i32)
35+
i32.const 0
36+
local.set $0
37+
block $break|0
38+
loop $continue|0
39+
local.get $0
40+
i32.const 255
41+
i32.and
42+
i32.const 10
43+
i32.eq
44+
if
45+
br $break|0
46+
end
47+
local.get $0
48+
i32.const 1
49+
i32.add
50+
local.tee $0
51+
i32.const 255
52+
i32.and
53+
br_if $continue|0
54+
end
55+
end
56+
)
57+
(func $loop-wrap/testSubsequentWrapped (; 2 ;) (type $FUNCSIG$vi) (param $0 i32)
58+
(local $1 i32)
59+
local.get $0
60+
local.set $1
61+
block $break|0
62+
loop $continue|0
63+
local.get $1
64+
i32.const 255
65+
i32.and
66+
i32.const 10
67+
i32.eq
68+
if
69+
br $break|0
70+
end
71+
local.get $1
72+
i32.const 1
73+
i32.add
74+
i32.const 255
75+
i32.and
76+
local.tee $1
77+
br_if $continue|0
78+
end
79+
end
80+
)
81+
(func $null (; 3 ;) (type $FUNCSIG$v)
82+
)
83+
)

0 commit comments

Comments
 (0)