wasm-split: Handle RefFuncs #6513
Conversation
src/ir/module-splitting.cpp
Outdated
| // Collect RefFuncs in a map from the function name to all RefFuncs that | ||
| // refer to it. We have one such map for the primary and secondary modules | ||
| // (that is, the primary map contains RefFuncs that are present in the | ||
| // primary module, and that refer to the secondary module, hence they are in | ||
| // need of fixing). | ||
| using Map = InsertOrderedMap<Name, std::vector<RefFunc*>>; | ||
| Map primaryMap, secondaryMap; |
There was a problem hiding this comment.
It would be best to handle secondary function references in the primary module and primary function references in the secondary module separately.
For primary-to-secondary references, these thunks are the best strategy, but for secondary-to-primary references, we almost don't need to do anything since the primary functions are imported into the secondary module and can be referred to directly. For the latter case, all you need to do is replace the TODO that was down in exportImportCalledPrimaryFunctions to collect referred-to primary functions as well.
For the former case, where we still need these thunks, it would be simpler to run this pass only on the primary module after moveSecondaryFunctions is called so you can assume that the parent function will always be a primary function.
| ;; This empty function is in the table. Just being present in the table is not | ||
| ;; enough of a reason for us to make a trampoline, even though in our IR the | ||
| ;; table is a list of ref.funcs. |
There was a problem hiding this comment.
How does the code differentiate RefFuncs in a table from other RefFuncs?
There was a problem hiding this comment.
Ah, this was a part I meant to write Friday evening and somehow forgot 😄 Added now + testing.
Co-authored-by: Thomas Lively <[email protected]>
src/ir/module-splitting.cpp
Outdated
| // Find all RefFuncs in elems, which we can ignore: tables are the means by | ||
| // which we connect the modules, and are handled directly. |
There was a problem hiding this comment.
Does this still work if the reference is in a passive element segment or an active element segment for a table other than the "active table" to which we are adding items?
There was a problem hiding this comment.
Ah, that shows my ignorance of wasm-split. Are all those not handled the same? I was assuming they are all kept in the primary module and that logic already existed to handle accessing them from secondary modules - is that not so?
There was a problem hiding this comment.
Taking a closer look at the code, it appears that we apply the same transformation to all active segments. There is a concerning TODO on line 149 ("TODO: Reject or handle passive element segments") that suggests we have pre-existing problems with passive segments.
I think we need to apply your thunk strategy to all secondary function references that appear in passive element segments.
There was a problem hiding this comment.
I see, yeah, looks like passive segments are not already handled. And yeah, it looks like we can handle them using this PR so I guess it makes sense to fix both here. I added that now.
The TODO on line 149 looks separate to me though? I suspect that is related to #6572
test/lit/wasm-split/ref.func.wast
Outdated
|
|
||
| (global $glob2 (ref func) (ref.func $second)) | ||
|
|
||
| (elem (i32.const 0) $in-table $second-in-table) |
There was a problem hiding this comment.
If you give this a name, its output line will be attached above it.
One of the first issues I see when fuzzing
wasm-split. This one looked easy to fix (famouslast words?) so I wrote up a quick patch.
When we have a
ref.func, if it refers to a function in the other module then make itrefer to a trampoline in this function instead. The trampoline does a direct call to the
target, and the normal
wasm-splitfixups handle getting that to work.