[wasm-split] Do not split out the start function

[kripken]

Like imports, automatically keep it in the primary module.

[aheejin]

Thanks! Can you handle it in multiSplitModule too?

[kripken]

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]

Why the order change?

[kripken]

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...

[tlively]

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

[aheejin]

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...

[kripken]

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)

[aheejin]

Can we just do something like

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

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

[kripken]

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]

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]

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

[aheejin]

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]

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;
	}