[wasm-split] Do not split out the start function by kripken · Pull Request #8711 · WebAssembly/binaryen

kripken · 2026-05-15T18:29:35Z

Like imports, automatically keep it in the primary module.

aheejin

Thanks! Can you handle it in multiSplitModule too?

kripken · 2026-05-15T19:52:25Z

Ok, I think I managed to do that. The first commit runs into a crash where the table is not present yet. I fixed that in the last commit by reordering operations: shareImportableItems needs the other code to run first so it can see which tables etc. need to be used.

The change to the existing test seems valid to me - now it is using the more accurate table definition from the primary module (which is larger).

However, I'm not very familiar with this code, so PTAL carefully!

aheejin · 2026-05-15T19:56:26Z

    indirectReferencesToSecondaryFunctions();
    indirectCallsToSecondaryFunctions();
    exportImportCalledPrimaryFunctions();
+    shareImportableItems();


Why the order change?

This is what I meant by this:

The first commit runs into a crash where the table is not present yet. I fixed that in the last commit by reordering operations: shareImportableItems needs the other code to run first so it can see which tables etc. need to be used.

That is the table needs to be created at the right time. I think...

I don't see why that would be related to this change 🤔

I don't either... Can you give an example?
That order change was the gist of #8688, so without breaking a lot of new assumptions, it is not easy to flip...

For an example, run the multi-split test in this PR. (That is, take all of this PR but the part in question. The test crashes because it doesn't find the table. Reordering makes the table exist - but I have no high-level understanding of the flow here 😄 so maybe it is the wrong fix)

Sorry, this was a separate bug. The fix is in #8728. After this, I think we can change this PR so that this filters the start functions only in wasm-split.cpp and I don't think we need to touch module-splitting.cpp. See #8711 (comment)

aheejin · 2026-05-15T20:09:08Z

+    // The start function must always be kept in the primary module.
    if (func->imported() || !configSecondaryFuncs.contains(func->name) ||
-        segmentReferrers.contains(func->name)) {
+        segmentReferrers.contains(func->name) || func->name == primary.start) {


Can we just do something like

if (primary.start) primaryFuncs.insert(primary.start)

at the start of the function, rather than in this loop?

We also need the if's else not to happen, though. That is, we need to add it in the primary and not in the secondary. If the loop body is not modified, it will add it to the second.

kripken · 2026-05-15T20:12:48Z

Looks like there are a bunch of tests that do not use the auto-updater, so I will need to update them manually. I did one in the last commit. I'll wait on the others to see if this fix is even the right one.

tlively · 2026-05-15T21:51:06Z

The fix looks right to me, but changing the order of splitting steps shouldn't be necessary.

aheejin · 2026-05-15T21:56:37Z

Looks like there are a bunch of tests that do not use the auto-updater, so I will need to update them manually.

I didn't use update_lit_checks.py in some tests because it doesn't work very well on split modules. For example, it somehow omits some module elements. But other module elements are still shown. Not sure what the exact rule is, but I guess update_lit_checks.py wasn't designed to match resulting module elements to existing module elements well when there are split modules.

And AI is fairly good at updating those expectations 😀 I often give prompts like "Don't change whether each test uses update_lit_checks.py or not, and update expectations for tests in test/lit/wasm-split"

But I also don't see why this PR should change existing tests' expectations, who don't have start function.

aheejin · 2026-05-15T22:32:19Z

+    if (function->name == wasm.start) {
+      if (!options.quiet) {
+        std::cerr << "warning: cannot split out start function " << func
+                  << "\n";
+      }
+      continue;
+    }


Can't you do the same thing in multiSplitModule after this line?

binaryen/src/tools/wasm-split/wasm-split.cpp

Line 437 in b5b9ebd

assert(currFuncs);

Then we don't need to do anything in module-splitting.cpp.

Also while you're at it you can add this there too, to be consistent with splitModule:

binaryen/src/tools/wasm-split/wasm-split.cpp

Lines 286 to 298 in b5b9ebd

if (!function) {

if (!options.quiet) {

std::cerr << "warning: function " << func << " does not exist\n";

}

continue;

}

if (function->imported()) {

if (!options.quiet) {

std::cerr << "warning: cannot split out imported function " << func

<< "\n";

}

continue;

}

Or even better, we can factor these three checks out to a function use it in both places

The way we currently share the active table with secondary modules https://github.com/WebAssembly/binaryen/blob/2c2509b5bfe399df400b2185caa370f56e563d32/src/ir/module-splitting.cpp#L1122-L1156 has missed an important case in multi-split. This is one of the unexpected consequences of WebAssembly#8688. Here the `secondary` module in this loop is a secondary module whose function have been put in the active table, mostly by `getTrampoline` function. When we have a call from `$A` to `$B` and `$B` is split, `getTrampoline` puts `$B` on the active table, which will be converted to a placeholder in `setupTablePatcing`. So the code above share the active table (and its base global, if exists) with `$B`'s module. The problem is we didn't share the active global with `$A`'s module. In a two-way split this is not a problem because `$A` is in the primary module. But in multi-split, `$A`'s module itself is another secondary module, and this module needs to access the active table because it should call the placeholder for `$B`. This does it in `indirectCallsToSecondaryFunctions`. --- I wish we can do the active table sharing for callee modules in `indirectCallsToSecondaryFunctions` too and get done with it, but we still need to share it in `shareImportableItems` for secondary modules that have functions to be replaced in the active table, because there is a case of existing secondary function names in the active table. See `KEEP-NONE` RUN line in the test below: https://github.com/WebAssembly/binaryen/blob/main/test/lit/wasm-split/basic.wast In this test, we reuse the existing table as the active table, which already has `$foo` in its element section. And that `foo` will be split. This won't be converted to trampolines because https://github.com/WebAssembly/binaryen/blob/5fcc1af12ae19032339ee5e56889e08b6912e8cb/src/ir/module-splitting.cpp#L947-L958 and because there is no trampoline, this doesn't get to have a call instruction from primary->secondary, so we can't handle it in `indirectCallsToSecondaryFunctions`. --- This fixes the error discussed in WebAssembly#8711 (comment).

The way we currently share the active table with secondary modules https://github.com/WebAssembly/binaryen/blob/2c2509b5bfe399df400b2185caa370f56e563d32/src/ir/module-splitting.cpp#L1122-L1156 has missed an important case in multi-split. This is one of the unexpected consequences of WebAssembly#8688. Here the `secondary` module in this loop is a secondary module whose functions have been put in the active table, mostly by `getTrampoline` function. When we have a call from `$A` to `$B` and `$B` is split, `getTrampoline` puts `$B` on the active table, which will be converted to a placeholder in `setupTablePatcing`. So the code above share the active table (and its base global, if exists) with `$B`'s module. The problem is we didn't share the active global with `$A`'s module. In a two-way split this is not a problem because `$A` is in the primary module. But in multi-split, `$A`'s module itself is another secondary module, and this module needs to access the active table because it should call the placeholder for `$B`. This does it in `indirectCallsToSecondaryFunctions`. --- I wish we can do the active table sharing for callee modules in `indirectCallsToSecondaryFunctions` too and get done with it, but we still need to share it in `shareImportableItems` for secondary modules that have functions to be replaced in the active table, because there is a case of existing secondary function names in the active table. See `KEEP-NONE` RUN line in the test below: https://github.com/WebAssembly/binaryen/blob/main/test/lit/wasm-split/basic.wast In this test, we reuse the existing table as the active table, which already has `$foo` in its element section. And that `foo` will be split. Because both `bar` and `foo` are split there is no cross-module call between them to process in `indirectCallsToSecondaryFunctions`. This won't be converted to trampolines because https://github.com/WebAssembly/binaryen/blob/5fcc1af12ae19032339ee5e56889e08b6912e8cb/src/ir/module-splitting.cpp#L947-L958 and because there is no trampoline, this doesn't get to have a call instruction from primary->secondary, but we still need to share the active table with the secondary module, and we can't handle it in `indirectCallsToSecondaryFunctions` --- This fixes the error discussed in WebAssembly#8711 (comment).

The way we currently share the active table with secondary modules https://github.com/WebAssembly/binaryen/blob/2c2509b5bfe399df400b2185caa370f56e563d32/src/ir/module-splitting.cpp#L1122-L1156 has missed an important case in multi-split. This is one of the unexpected consequences of #8688. Here the `secondary` module in this loop is a secondary module whose functions have been put in the active table, mostly by `getTrampoline` function. When we have a call from `$A` to `$B` and `$B` is split, `getTrampoline` puts `$B` on the active table, which will be converted to a placeholder in `setupTablePatcing`. So the code above share the active table (and its base global, if exists) with `$B`'s module. The problem is we didn't share the active global with `$A`'s module. In a two-way split this is not a problem because `$A` is in the primary module. But in multi-split, `$A`'s module itself is another secondary module, and this module needs to access the active table because it should call the placeholder for `$B`. This does it in `indirectCallsToSecondaryFunctions`. --- I wish we can do the active table sharing for callee modules in `indirectCallsToSecondaryFunctions` too and get done with it, but we still need to share it in `shareImportableItems` for secondary modules that have functions to be replaced in the active table, because there is a case of existing secondary function names in the active table. See `KEEP-NONE` RUN line in the test below: https://github.com/WebAssembly/binaryen/blob/main/test/lit/wasm-split/basic.wast In this test, we reuse the existing table as the active table, which already has `$foo` in its element section. And that `foo` will be split. Because both `bar` and `foo` are split there is no cross-module call between them to process in `indirectCallsToSecondaryFunctions`. This won't be converted to trampolines because https://github.com/WebAssembly/binaryen/blob/5fcc1af12ae19032339ee5e56889e08b6912e8cb/src/ir/module-splitting.cpp#L947-L958 and because there is no trampoline, this doesn't get to have a call instruction from primary->secondary, but we still need to share the active table with the secondary module, and we can't handle it in `indirectCallsToSecondaryFunctions`. --- This fixes the error discussed in #8711 (comment).

fix

306e134

kripken requested a review from a team as a code owner May 15, 2026 18:29

kripken requested review from aheejin and removed request for a team May 15, 2026 18:29

aheejin reviewed May 15, 2026

View reviewed changes

kripken added 2 commits May 15, 2026 11:56

try to fix multi-split

969ba9b

fix

ac36874

fix tests

6aabdf3

aheejin reviewed May 15, 2026

View reviewed changes

sort lines in existing test

ff9cfab

aheejin reviewed May 15, 2026

View reviewed changes

aheejin mentioned this pull request May 19, 2026

[wasm-split] Share active table with caller modules #8728

Merged

	if (!function) {
	if (!options.quiet) {
	std::cerr << "warning: function " << func << " does not exist\n";
	}
	continue;
	}
	if (function->imported()) {
	if (!options.quiet) {
	std::cerr << "warning: cannot split out imported function " << func
	<< "\n";
	}
	continue;
	}

Conversation

kripken commented May 15, 2026

Uh oh!

aheejin left a comment

Choose a reason for hiding this comment

Uh oh!

kripken commented May 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aheejin May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aheejin May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented May 15, 2026

Uh oh!

tlively commented May 15, 2026

Uh oh!

aheejin commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aheejin May 15, 2026 •

edited

Loading

aheejin May 19, 2026 •

edited

Loading

aheejin commented May 15, 2026 •

edited

Loading