Skip to content

Commit eb1f90a

Browse files
kripkenkripken
authored
GUFA: Assume nothing about the contents of public tables (#7234)
Imported and exported tables may get new items from the outside, so we cannot infer values there. This uncovered a bug with doing addRoot on tuples, which is also fixed here. Also a tiny fix to debug logging.
1 parent a1e8b7c commit eb1f90a

File tree

4 files changed

+189
-9
lines changed

4 files changed

+189
-9
lines changed

‎src/ir/possible-contents.cpp‎

Lines changed: 49 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -468,6 +468,12 @@ namespace wasm {
468468

469469
namespace {
470470

471+
// Information that is shared with InfoCollector.
472+
struct SharedInfo {
473+
// The names of tables that are imported or exported.
474+
std::unordered_set<Name> publicTables;
475+
};
476+
471477
// The data we gather from each function, as we process them in parallel. Later
472478
// this will be merged into a single big graph.
473479
struct CollectedFuncInfo {
@@ -533,11 +539,14 @@ struct BreakTargetWalker : public PostWalker<SubType, VisitorType> {
533539
// main flow will begin.
534540
struct InfoCollector
535541
: public BreakTargetWalker<InfoCollector, OverriddenVisitor<InfoCollector>> {
542+
SharedInfo& shared;
536543
CollectedFuncInfo& info;
537544
const PassOptions& options;
538545

539-
InfoCollector(CollectedFuncInfo& info, const PassOptions& options)
540-
: info(info), options(options) {}
546+
InfoCollector(SharedInfo& shared,
547+
CollectedFuncInfo& info,
548+
const PassOptions& options)
549+
: shared(shared), info(info), options(options) {}
541550

542551
// Check if a type is relevant for us. If not, we can ignore it entirely.
543552
bool isRelevant(Type type) {
@@ -888,9 +897,16 @@ struct InfoCollector
888897
curr->operands.push_back(target);
889898
}
890899
void visitCallIndirect(CallIndirect* curr) {
891-
// TODO: the table identity could also be used here
892900
// TODO: optimize the call target like CallRef
893901
handleIndirectCall(curr, curr->heapType);
902+
903+
// If this goes to a public table, then we must root the output, as the
904+
// table could contain anything at all, and calling functions there could
905+
// return anything at all.
906+
if (shared.publicTables.count(curr->table)) {
907+
addRoot(curr);
908+
}
909+
// TODO: the table identity could also be used here in more ways
894910
}
895911
void visitCallRef(CallRef* curr) {
896912
handleIndirectCall(curr, curr->target->type);
@@ -1375,7 +1391,15 @@ struct InfoCollector
13751391
if (contents.isMany()) {
13761392
contents = PossibleContents::fromType(curr->type);
13771393
}
1378-
addRoot(ExpressionLocation{curr, 0}, contents);
1394+
1395+
if (!curr->type.isTuple()) {
1396+
addRoot(ExpressionLocation{curr, 0}, contents);
1397+
} else {
1398+
// For a tuple, we create a root for each index.
1399+
for (Index i = 0; i < curr->type.size(); i++) {
1400+
addRoot(ExpressionLocation{curr, i}, contents.getTupleItem(i));
1401+
}
1402+
}
13791403
}
13801404
}
13811405

@@ -2121,10 +2145,26 @@ Flower::Flower(Module& wasm, const PassOptions& options)
21212145
std::cout << "parallel phase\n";
21222146
#endif
21232147

2124-
// First, collect information from each function.
2148+
// Compute shared info that we need for the main pass over each function, such
2149+
// as the imported/exported tables.
2150+
SharedInfo shared;
2151+
2152+
for (auto& table : wasm.tables) {
2153+
if (table->imported()) {
2154+
shared.publicTables.insert(table->name);
2155+
}
2156+
}
2157+
2158+
for (auto& ex : wasm.exports) {
2159+
if (ex->kind == ExternalKind::Table) {
2160+
shared.publicTables.insert(ex->value);
2161+
}
2162+
}
2163+
2164+
// Collect information from each function.
21252165
ModuleUtils::ParallelFunctionAnalysis<CollectedFuncInfo> analysis(
21262166
wasm, [&](Function* func, CollectedFuncInfo& info) {
2127-
InfoCollector finder(info, options);
2167+
InfoCollector finder(shared, info, options);
21282168

21292169
if (func->imported()) {
21302170
// Imports return unknown values.
@@ -2146,7 +2186,7 @@ Flower::Flower(Module& wasm, const PassOptions& options)
21462186
// Also walk the global module code (for simplicity, also add it to the
21472187
// function map, using a "function" key of nullptr).
21482188
auto& globalInfo = analysis.map[nullptr];
2149-
InfoCollector finder(globalInfo, options);
2189+
InfoCollector finder(shared, globalInfo, options);
21502190
finder.walkModuleCode(&wasm);
21512191

21522192
#ifdef POSSIBLE_CONTENTS_DEBUG
@@ -2944,8 +2984,8 @@ void Flower::dump(Location location) {
29442984
std::cout << " globalloc " << loc->name << '\n';
29452985
} else if (std::get_if<SignatureParamLocation>(&location)) {
29462986
std::cout << " sigparamloc " << '\n';
2947-
} else if (std::get_if<SignatureResultLocation>(&location)) {
2948-
std::cout << " sigresultloc " << '\n';
2987+
} else if (auto* loc = std::get_if<SignatureResultLocation>(&location)) {
2988+
std::cout << " sigresultloc " << loc->type << " : " << loc->index << '\n';
29492989
} else if (auto* loc = std::get_if<NullLocation>(&location)) {
29502990
std::cout << " Nullloc " << loc->type << '\n';
29512991
} else {

‎src/ir/possible-contents.h‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,25 @@ class PossibleContents {
301301
}
302302
}
303303

304+
// For possible contents of a tuple type, get one of the items.
305+
PossibleContents getTupleItem(Index i) {
306+
auto type = getType();
307+
assert(type.isTuple());
308+
if (std::get_if<Literal>(&value)) {
309+
WASM_UNREACHABLE("TODO: use Literals");
310+
} else if (std::get_if<GlobalInfo>(&value)) {
311+
WASM_UNREACHABLE("TODO");
312+
} else if (auto* cone = std::get_if<ConeType>(&value)) {
313+
// Return a full cone of the appropriate type, as we lack depth info for
314+
// the separate items in the tuple (tuples themselves have no subtyping,
315+
// so the tuple's depth must be 0, i.e., an exact type).
316+
assert(cone->depth == 0);
317+
return fullConeType(type[i]);
318+
} else {
319+
WASM_UNREACHABLE("not a tuple");
320+
}
321+
}
322+
304323
size_t hash() const {
305324
// First hash the index of the variant, then add the internals for each.
306325
size_t ret = std::hash<size_t>()(value.index());

‎test/gtest/possible-contents.cpp‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -948,3 +948,15 @@ TEST_F(PossibleContentsTest, TestOracleNoFullCones) {
948948
ASSERT_TRUE(bodyContents.isConeType());
949949
EXPECT_EQ(bodyContents.getCone().depth, Index(2));
950950
}
951+
952+
TEST_F(PossibleContentsTest, TestTupleItems) {
953+
// All tuples must be exact (there is no subtyping for tuples).
954+
Type funcref = Type(HeapType::func, Nullable);
955+
auto tupleType = Type({Type::i32, funcref});
956+
PossibleContents tuple = PossibleContents::exactType(tupleType);
957+
958+
// We can get the tuple items. The funcref is a full cone, as we did not have
959+
// depth info for it.
960+
EXPECT_EQ(tuple.getTupleItem(0), PossibleContents::fullConeType(Type::i32));
961+
EXPECT_EQ(tuple.getTupleItem(1), PossibleContents::fullConeType(funcref));
962+
}

‎test/lit/passes/gufa-tables.wast‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
2+
;; RUN: foreach %s %t wasm-opt -all --gufa -S -o - | filecheck %s
3+
4+
;; Non-private tables can contain anything, so we cannot assume anything about
5+
;; the results of a call to them.
6+
(module
7+
;; CHECK: (type $type (func (result f64)))
8+
(type $type (func (result f64)))
9+
10+
;; CHECK: (type $1 (func))
11+
12+
;; CHECK: (import "a" "b" (table $imported 30 40 funcref))
13+
(import "a" "b" (table $imported 30 40 funcref))
14+
15+
;; CHECK: (table $exported 10 20 funcref)
16+
(table $exported 10 20 funcref)
17+
18+
;; CHECK: (table $private 50 60 funcref)
19+
(table $private 50 60 funcref)
20+
21+
;; CHECK: (export "func" (func $func))
22+
23+
;; CHECK: (export "table" (table $exported))
24+
(export "table" (table $exported))
25+
26+
;; CHECK: (func $func (type $1)
27+
;; CHECK-NEXT: (drop
28+
;; CHECK-NEXT: (call_indirect $exported (type $type)
29+
;; CHECK-NEXT: (i32.const 0)
30+
;; CHECK-NEXT: )
31+
;; CHECK-NEXT: )
32+
;; CHECK-NEXT: (drop
33+
;; CHECK-NEXT: (call_indirect $imported (type $type)
34+
;; CHECK-NEXT: (i32.const 0)
35+
;; CHECK-NEXT: )
36+
;; CHECK-NEXT: )
37+
;; CHECK-NEXT: (drop
38+
;; CHECK-NEXT: (block
39+
;; CHECK-NEXT: (drop
40+
;; CHECK-NEXT: (call_indirect $private (type $type)
41+
;; CHECK-NEXT: (i32.const 0)
42+
;; CHECK-NEXT: )
43+
;; CHECK-NEXT: )
44+
;; CHECK-NEXT: (unreachable)
45+
;; CHECK-NEXT: )
46+
;; CHECK-NEXT: )
47+
;; CHECK-NEXT: )
48+
(func $func (export "func")
49+
;; We cannot optimize anything with the exported or imported table.
50+
(drop
51+
(call_indirect $exported (type $type)
52+
(i32.const 0)
53+
)
54+
)
55+
(drop
56+
(call_indirect $imported (type $type)
57+
(i32.const 0)
58+
)
59+
)
60+
;; We can optimize the private table: this will trap.
61+
(drop
62+
(call_indirect $private (type $type)
63+
(i32.const 0)
64+
)
65+
)
66+
)
67+
)
68+
69+
;; Test a tuple-returning call_indirect. There is nothing to optimize here, but
70+
;; we should not error while marking the call_indirect as returning a tuple of
71+
;; unknown values (unknown, since the table is exported).
72+
(module
73+
;; CHECK: (type $5 (func (result f64 i32)))
74+
(type $5 (func (result f64 i32)))
75+
76+
;; CHECK: (table $table 44 funcref)
77+
(table $table 44 funcref)
78+
79+
;; CHECK: (elem $elem (i32.const 0) $make)
80+
(elem $elem (i32.const 0) $make)
81+
82+
;; CHECK: (export "table" (table $table))
83+
(export "table" (table $table))
84+
85+
;; CHECK: (func $make (type $5) (result f64 i32)
86+
;; CHECK-NEXT: (tuple.make 2
87+
;; CHECK-NEXT: (f64.const 3.14159)
88+
;; CHECK-NEXT: (i32.const 42)
89+
;; CHECK-NEXT: )
90+
;; CHECK-NEXT: )
91+
(func $make (type $5) (result f64 i32)
92+
(tuple.make 2
93+
(f64.const 3.14159)
94+
(i32.const 42)
95+
)
96+
)
97+
98+
;; CHECK: (func $call (type $5) (result f64 i32)
99+
;; CHECK-NEXT: (call_indirect $table (type $5)
100+
;; CHECK-NEXT: (i32.const 0)
101+
;; CHECK-NEXT: )
102+
;; CHECK-NEXT: )
103+
(func $call (result f64 i32)
104+
(call_indirect $table (type $5)
105+
(i32.const 0)
106+
)
107+
)
108+
)
109+

0 commit comments

Comments
 (0)