Skip to content

Commit d675fbf

Browse files
kripkenkripken
authored
table-utils: Handle table.grow (#8671)
This instruction was just not handled. That was mostly ok, as growth only appends, but we did miscompile in some cases: if we call a high index in the table, growth might save us from trapping, but we assumed we still trap in Directize.
1 parent 1abcee9 commit d675fbf

6 files changed

Lines changed: 262 additions & 27 deletions

File tree

‎src/ir/table-utils.cpp‎

Lines changed: 31 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -95,21 +95,21 @@ TableInfoMap computeTableInfo(Module& wasm, bool initialContentsImmutable) {
9595

9696
for (auto& table : wasm.tables) {
9797
if (table->imported()) {
98-
tables[table->name].mayBeModified = true;
98+
tables[table->name].hasSet = true;
9999
}
100100
}
101101

102102
for (auto& ex : wasm.exports) {
103103
if (ex->kind == ExternalKind::Table) {
104-
tables[*ex->getInternalName()].mayBeModified = true;
104+
tables[*ex->getInternalName()].hasSet = true;
105105
}
106106
}
107107

108108
// Find which tables have sets, by scanning for instructions. Only do so if we
109109
// might learn anything new.
110110
auto hasUnmodifiableTable = false;
111111
for (auto& [_, info] : tables) {
112-
if (!info.mayBeModified) {
112+
if (!info.hasSet) {
113113
hasUnmodifiableTable = true;
114114
break;
115115
}
@@ -118,39 +118,54 @@ TableInfoMap computeTableInfo(Module& wasm, bool initialContentsImmutable) {
118118
return tables;
119119
}
120120

121-
using TablesWithSet = std::unordered_set<Name>;
121+
// Miniature form of TableInfo, without things we don't need (some of which
122+
// cause compilation errors on the copies below).
123+
struct MiniTableInfo {
124+
bool hasSet = false;
125+
bool hasGrow = false;
126+
};
122127

123-
ModuleUtils::ParallelFunctionAnalysis<TablesWithSet> analysis(
124-
wasm, [&](Function* func, TablesWithSet& tablesWithSet) {
128+
using MiniTableInfoMap = std::unordered_map<Name, MiniTableInfo>;
129+
130+
ModuleUtils::ParallelFunctionAnalysis<MiniTableInfoMap> analysis(
131+
wasm, [&](Function* func, MiniTableInfoMap& tableInfoMap) {
125132
if (func->imported()) {
126133
return;
127134
}
128135

129136
struct Finder : public PostWalker<Finder> {
130-
TablesWithSet& tablesWithSet;
137+
MiniTableInfoMap& tableInfoMap;
131138

132-
Finder(TablesWithSet& tablesWithSet) : tablesWithSet(tablesWithSet) {}
139+
Finder(MiniTableInfoMap& tableInfoMap) : tableInfoMap(tableInfoMap) {}
133140

134141
void visitTableSet(TableSet* curr) {
135-
tablesWithSet.insert(curr->table);
142+
tableInfoMap[curr->table].hasSet = true;
136143
}
137144
void visitTableFill(TableFill* curr) {
138-
tablesWithSet.insert(curr->table);
145+
tableInfoMap[curr->table].hasSet = true;
139146
}
140147
void visitTableCopy(TableCopy* curr) {
141-
tablesWithSet.insert(curr->destTable);
148+
tableInfoMap[curr->destTable].hasSet = true;
142149
}
143150
void visitTableInit(TableInit* curr) {
144-
tablesWithSet.insert(curr->table);
151+
tableInfoMap[curr->table].hasSet = true;
152+
}
153+
void visitTableGrow(TableGrow* curr) {
154+
tableInfoMap[curr->table].hasGrow = true;
145155
}
146156
};
147157

148-
Finder(tablesWithSet).walkFunction(func);
158+
Finder(tableInfoMap).walkFunction(func);
149159
});
150160

151-
for (auto& [_, names] : analysis.map) {
152-
for (auto name : names) {
153-
tables[name].mayBeModified = true;
161+
for (auto& [_, tableInfoMap] : analysis.map) {
162+
for (auto& [tableName, info] : tableInfoMap) {
163+
if (info.hasSet) {
164+
tables[tableName].hasSet = true;
165+
}
166+
if (info.hasGrow) {
167+
tables[tableName].hasGrow = true;
168+
}
154169
}
155170
}
156171

‎src/ir/table-utils.h‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -122,9 +122,14 @@ bool usesExpressions(ElementSegment* curr, Module* module);
122122

123123
// Information about a table's optimizability.
124124
struct TableInfo {
125-
// Whether the table may be modified at runtime, either because it is imported
126-
// or exported, or table.set operations exist for it in the code.
127-
bool mayBeModified = false;
125+
// Whether the table has writes to it (anything but a grow, see below). The
126+
// writes may be internal, or through imports and exports.
127+
bool hasSet = false;
128+
129+
// Whether the table may grow. Growing does modify the table, but it only
130+
// appends, so we track this separately from mayBeModified. This allows more
131+
// optimizations in tables that grow but have no other sets.
132+
bool hasGrow = false;
128133

129134
// Whether we can assume that the initial contents are immutable. That is, if
130135
// a table looks like [a, b, c] in the wasm, and we see a call to index 1, we
@@ -144,6 +149,9 @@ struct TableInfo {
144149

145150
std::unique_ptr<TableUtils::FlatTable> flatTable;
146151

152+
// Whether the contents may change.
153+
bool mayBeModified() const { return hasSet || hasGrow; }
154+
147155
// Whether we can optimize using this table's data on the entry level, that
148156
// is, individual entries in the table are known to us, so calls through the
149157
// table with known indexes can be inferred, etc.
@@ -154,7 +162,10 @@ struct TableInfo {
154162
// contents, even if other things might be appended later, which we
155163
// cannot infer).
156164
// * The table is flat (so we can see what is in it, by index).
157-
return (!mayBeModified || initialContentsImmutable) && flatTable->valid;
165+
//
166+
// Note that we do not check hasGrow, as we can optimize at least *some*
167+
// entries in that case (growth only appends).
168+
return (!hasSet || initialContentsImmutable) && flatTable->valid;
158169
}
159170
};
160171

‎src/passes/Directize.cpp‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -127,18 +127,23 @@ struct FunctionDirectizer : public WalkerPass<PostWalker<FunctionDirectizer>> {
127127
// The index is out of bounds for the initial table's content. This may
128128
// trap, but it may also not trap if the table is modified later (if a
129129
// function is appended to it).
130-
if (!table.mayBeModified) {
130+
if (!table.mayBeModified()) {
131131
return CallUtils::Trap{};
132132
} else {
133133
// The table may be modified, so it might be appended to. We should only
134-
// get here in the case that the initial contents are immutable, as
135-
// otherwise we have nothing to optimize at all.
136-
assert(table.initialContentsImmutable);
134+
// get here in the case that the initial contents are immutable, or the
135+
// table can grow, as otherwise we have nothing to optimize at all.
136+
assert(table.initialContentsImmutable || table.hasGrow);
137137
return CallUtils::Unknown{};
138138
}
139139
}
140140
auto name = flatTable.names[index];
141141
if (!name.is()) {
142+
// No segment wrote to this part of the initial contents of the table.
143+
// This must trap, as we only get here if we can optimize such cases,
144+
// relying on the fact that the table cannot be modified, or at least the
145+
// initial contents cannot be.
146+
assert(!table.hasSet || table.initialContentsImmutable);
142147
return CallUtils::Trap{};
143148
}
144149
auto* func = getModule()->getFunction(name);

‎src/passes/RemoveUnusedModuleElements.cpp‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -432,11 +432,13 @@ struct Analyzer {
432432

433433
// Note a possible call of a function reference as well, if something else
434434
// might be written into the table during runtime.
435-
// TODO: Add an option for immutable initial content like Directize?
435+
// TODO: Add an option for immutable initial content like Directize? Can
436+
// also check for grow without set, which leaves initial entries
437+
// fixed.
436438
if (!tableInfoMap) {
437439
tableInfoMap = TableUtils::computeTableInfo(*module);
438440
}
439-
if ((*tableInfoMap)[table].mayBeModified) {
441+
if ((*tableInfoMap)[table].mayBeModified()) {
440442
useCallRefType(type);
441443
}
442444
}

‎test/lit/passes/directize_all-features.wast‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1953,3 +1953,92 @@
19531953
)
19541954
)
19551955

1956+
;; table.grow inhibits some optimizations.
1957+
(module
1958+
;; CHECK: (type $func (func))
1959+
;; IMMUT: (type $func (func))
1960+
(type $func (func))
1961+
1962+
;; CHECK: (table $table 5 funcref)
1963+
;; IMMUT: (table $table 5 funcref)
1964+
(table $table 5 funcref)
1965+
1966+
;; CHECK: (elem $table (i32.const 1) $target)
1967+
;; IMMUT: (elem $table (i32.const 1) $target)
1968+
(elem $table (i32.const 1) $target)
1969+
1970+
;; CHECK: (elem declare func $grow)
1971+
1972+
;; CHECK: (export "caller" (func $caller))
1973+
1974+
;; CHECK: (func $grow (type $func)
1975+
;; CHECK-NEXT: (drop
1976+
;; CHECK-NEXT: (table.grow $table
1977+
;; CHECK-NEXT: (ref.func $grow)
1978+
;; CHECK-NEXT: (i32.const 42)
1979+
;; CHECK-NEXT: )
1980+
;; CHECK-NEXT: )
1981+
;; CHECK-NEXT: )
1982+
;; IMMUT: (elem declare func $grow)
1983+
1984+
;; IMMUT: (export "caller" (func $caller))
1985+
1986+
;; IMMUT: (func $grow (type $func)
1987+
;; IMMUT-NEXT: (drop
1988+
;; IMMUT-NEXT: (table.grow $table
1989+
;; IMMUT-NEXT: (ref.func $grow)
1990+
;; IMMUT-NEXT: (i32.const 42)
1991+
;; IMMUT-NEXT: )
1992+
;; IMMUT-NEXT: )
1993+
;; IMMUT-NEXT: )
1994+
(func $grow
1995+
(drop
1996+
(table.grow $table
1997+
(ref.func $grow)
1998+
(i32.const 42)
1999+
)
2000+
)
2001+
)
2002+
2003+
;; CHECK: (func $caller (type $func)
2004+
;; CHECK-NEXT: (call $target)
2005+
;; CHECK-NEXT: (call_indirect $table (type $func)
2006+
;; CHECK-NEXT: (i32.const 10)
2007+
;; CHECK-NEXT: )
2008+
;; CHECK-NEXT: (call_indirect $table (type $func)
2009+
;; CHECK-NEXT: (i32.const 1000)
2010+
;; CHECK-NEXT: )
2011+
;; CHECK-NEXT: )
2012+
;; IMMUT: (func $caller (type $func)
2013+
;; IMMUT-NEXT: (call $target)
2014+
;; IMMUT-NEXT: (call_indirect $table (type $func)
2015+
;; IMMUT-NEXT: (i32.const 10)
2016+
;; IMMUT-NEXT: )
2017+
;; IMMUT-NEXT: (call_indirect $table (type $func)
2018+
;; IMMUT-NEXT: (i32.const 1000)
2019+
;; IMMUT-NEXT: )
2020+
;; IMMUT-NEXT: )
2021+
(func $caller (export "caller")
2022+
;; This is in the elem segment, so we can optimize it. Growth can only append.
2023+
(call_indirect (type $func)
2024+
(i32.const 1)
2025+
)
2026+
;; This is in the range that we grow to, if grow() is called, but we don't
2027+
;; know if it will, so we don't optimize.
2028+
(call_indirect (type $func)
2029+
(i32.const 10)
2030+
)
2031+
;; This is in the range that we grow to, if grow() is called multiple times,
2032+
;; but again we can't optimize.
2033+
(call_indirect (type $func)
2034+
(i32.const 1000)
2035+
)
2036+
)
2037+
2038+
;; CHECK: (func $target (type $func)
2039+
;; CHECK-NEXT: )
2040+
;; IMMUT: (func $target (type $func)
2041+
;; IMMUT-NEXT: )
2042+
(func $target
2043+
)
2044+
)

‎test/lit/passes/remove-unused-module-elements-tables.wast‎

Lines changed: 114 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@
333333
)
334334
)
335335

336-
;; As above, but now the table has an initialistion expression
336+
;; As above, but now the table has an initialization expression
337337
(module
338338
(rec
339339
;; CHECK: (rec
@@ -447,3 +447,116 @@
447447
(drop (i32.const 30))
448448
)
449449
)
450+
451+
;; As above, but now the table has a table.grow. Like table.set, this prevents
452+
;; optimization.
453+
(module
454+
(rec
455+
;; CHECK: (rec
456+
;; CHECK-NEXT: (type $foo (func))
457+
;; OPEN_WORLD: (rec
458+
;; OPEN_WORLD-NEXT: (type $foo (func))
459+
(type $foo (func))
460+
;; CHECK: (type $bar (func))
461+
;; OPEN_WORLD: (type $bar (func))
462+
(type $bar (func))
463+
)
464+
465+
;; CHECK: (type $2 (func))
466+
467+
;; CHECK: (table $table 10 funcref)
468+
;; OPEN_WORLD: (type $2 (func))
469+
470+
;; OPEN_WORLD: (table $table 10 funcref)
471+
(table $table 10 funcref)
472+
;; CHECK: (elem $table (i32.const 0) $foo-in-table $bar)
473+
;; OPEN_WORLD: (elem $table (i32.const 0) $foo-in-table $bar)
474+
(elem $table (i32.const 0) $foo-in-table $bar)
475+
476+
;; CHECK: (elem declare func $foo-not-in-table)
477+
478+
;; CHECK: (export "export" (func $export))
479+
480+
;; CHECK: (func $export (type $2)
481+
;; CHECK-NEXT: (call_indirect $table (type $foo)
482+
;; CHECK-NEXT: (i32.const 5)
483+
;; CHECK-NEXT: )
484+
;; CHECK-NEXT: (drop
485+
;; CHECK-NEXT: (table.grow $table
486+
;; CHECK-NEXT: (ref.func $foo-not-in-table)
487+
;; CHECK-NEXT: (i32.const 7)
488+
;; CHECK-NEXT: )
489+
;; CHECK-NEXT: )
490+
;; CHECK-NEXT: )
491+
;; OPEN_WORLD: (elem declare func $foo-not-in-table)
492+
493+
;; OPEN_WORLD: (export "export" (func $export))
494+
495+
;; OPEN_WORLD: (func $export (type $2)
496+
;; OPEN_WORLD-NEXT: (call_indirect $table (type $foo)
497+
;; OPEN_WORLD-NEXT: (i32.const 5)
498+
;; OPEN_WORLD-NEXT: )
499+
;; OPEN_WORLD-NEXT: (drop
500+
;; OPEN_WORLD-NEXT: (table.grow $table
501+
;; OPEN_WORLD-NEXT: (ref.func $foo-not-in-table)
502+
;; OPEN_WORLD-NEXT: (i32.const 7)
503+
;; OPEN_WORLD-NEXT: )
504+
;; OPEN_WORLD-NEXT: )
505+
;; OPEN_WORLD-NEXT: )
506+
(func $export (export "export")
507+
(call_indirect $table (type $foo)
508+
(i32.const 5)
509+
)
510+
(drop
511+
(table.grow $table
512+
(ref.func $foo-not-in-table)
513+
(i32.const 7)
514+
)
515+
)
516+
)
517+
518+
;; CHECK: (func $foo-in-table (type $foo)
519+
;; CHECK-NEXT: (drop
520+
;; CHECK-NEXT: (i32.const 10)
521+
;; CHECK-NEXT: )
522+
;; CHECK-NEXT: )
523+
;; OPEN_WORLD: (func $foo-in-table (type $foo)
524+
;; OPEN_WORLD-NEXT: (drop
525+
;; OPEN_WORLD-NEXT: (i32.const 10)
526+
;; OPEN_WORLD-NEXT: )
527+
;; OPEN_WORLD-NEXT: )
528+
(func $foo-in-table (type $foo)
529+
;; This is in the table, and might be reached.
530+
(drop (i32.const 10))
531+
)
532+
533+
;; CHECK: (func $foo-not-in-table (type $foo)
534+
;; CHECK-NEXT: (drop
535+
;; CHECK-NEXT: (i32.const 20)
536+
;; CHECK-NEXT: )
537+
;; CHECK-NEXT: )
538+
;; OPEN_WORLD: (func $foo-not-in-table (type $foo)
539+
;; OPEN_WORLD-NEXT: (drop
540+
;; OPEN_WORLD-NEXT: (i32.const 20)
541+
;; OPEN_WORLD-NEXT: )
542+
;; OPEN_WORLD-NEXT: )
543+
(func $foo-not-in-table (type $foo)
544+
;; The reference taken of this function might be added to the table using
545+
;; table.grow, so we can do nothing here.
546+
(drop (i32.const 20))
547+
)
548+
549+
;; CHECK: (func $bar (type $bar)
550+
;; CHECK-NEXT: (unreachable)
551+
;; CHECK-NEXT: )
552+
;; OPEN_WORLD: (func $bar (type $bar)
553+
;; OPEN_WORLD-NEXT: (drop
554+
;; OPEN_WORLD-NEXT: (i32.const 30)
555+
;; OPEN_WORLD-NEXT: )
556+
;; OPEN_WORLD-NEXT: )
557+
(func $bar (type $bar)
558+
;; Not even references, so this is unreachable in closed world.
559+
(drop (i32.const 30))
560+
)
561+
)
562+

0 commit comments

Comments
 (0)