-
Notifications
You must be signed in to change notification settings - Fork 836
wasm-split: Handle RefFuncs #6513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 10 commits
d2eef23
c3338f3
9153f0b
9a3e84f
a4dc3d4
422ad86
15ca0a5
1f81117
bc24e02
b2b93b4
8e972b1
591f4d5
bd36f82
a0ffac6
910f01b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| ;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. | ||
|
|
||
| ;; RUN: wasm-split %s --split-funcs=second,second-in-table -g -o1 %t.1.wasm -o2 %t.2.wasm -all | filecheck %s | ||
| ;; RUN: wasm-dis %t.1.wasm | filecheck %s --check-prefix PRIMARY | ||
| ;; RUN: wasm-dis %t.2.wasm | filecheck %s --check-prefix SECONDARY | ||
|
|
||
| ;; Test that we handle ref.func operations properly as we split out $second. | ||
| ;; ref.funcs that refer to the other module must be fixed up to refer to | ||
| ;; something in the same module, that then trampolines to the other. | ||
| (module | ||
| ;; PRIMARY: (type $0 (func)) | ||
|
|
||
| ;; PRIMARY: (import "placeholder" "1" (func $placeholder_1)) | ||
|
|
||
| ;; PRIMARY: (import "placeholder" "2" (func $placeholder_2)) | ||
|
|
||
| ;; PRIMARY: (global $glob1 (ref func) (ref.func $prime)) | ||
|
|
||
| ;; PRIMARY: (global $glob2 (ref func) (ref.func $2)) | ||
|
|
||
| ;; PRIMARY: (table $table 3 3 funcref) | ||
| (table $table 1 1 funcref) | ||
|
|
||
| (global $glob1 (ref func) (ref.func $prime)) | ||
|
|
||
| (global $glob2 (ref func) (ref.func $second)) | ||
|
|
||
| (elem (i32.const 0) $in-table $second-in-table) | ||
|
||
|
|
||
| ;; PRIMARY: (elem $0 (i32.const 0) $in-table $placeholder_1 $placeholder_2) | ||
|
|
||
| ;; PRIMARY: (export "prime" (func $prime)) | ||
|
|
||
| ;; PRIMARY: (export "table" (table $table)) | ||
|
|
||
| ;; PRIMARY: (export "global" (global $glob1)) | ||
|
|
||
| ;; PRIMARY: (export "global_3" (global $glob2)) | ||
|
|
||
| ;; PRIMARY: (func $prime | ||
| ;; PRIMARY-NEXT: (drop | ||
| ;; PRIMARY-NEXT: (ref.func $prime) | ||
| ;; PRIMARY-NEXT: ) | ||
| ;; PRIMARY-NEXT: (drop | ||
| ;; PRIMARY-NEXT: (ref.func $2) | ||
| ;; PRIMARY-NEXT: ) | ||
| ;; PRIMARY-NEXT: ) | ||
| (func $prime | ||
| (drop | ||
| (ref.func $prime) | ||
| ) | ||
| (drop | ||
| (ref.func $second) | ||
| ) | ||
| ) | ||
|
|
||
| ;; SECONDARY: (type $0 (func)) | ||
|
|
||
| ;; SECONDARY: (import "primary" "table" (table $table 3 3 funcref)) | ||
|
|
||
| ;; SECONDARY: (import "primary" "global" (global $glob1 (ref func))) | ||
|
|
||
| ;; SECONDARY: (import "primary" "global_3" (global $glob2 (ref func))) | ||
|
|
||
| ;; SECONDARY: (import "primary" "prime" (func $prime)) | ||
|
|
||
| ;; SECONDARY: (elem $0 (i32.const 1) $second-in-table $second) | ||
|
|
||
| ;; SECONDARY: (func $second | ||
| ;; SECONDARY-NEXT: (drop | ||
| ;; SECONDARY-NEXT: (ref.func $prime) | ||
| ;; SECONDARY-NEXT: ) | ||
| ;; SECONDARY-NEXT: (drop | ||
| ;; SECONDARY-NEXT: (ref.func $second) | ||
| ;; SECONDARY-NEXT: ) | ||
| ;; SECONDARY-NEXT: ) | ||
| (func $second | ||
| (drop | ||
| (ref.func $prime) | ||
| ) | ||
| (drop | ||
| (ref.func $second) | ||
| ) | ||
| ) | ||
|
|
||
| ;; PRIMARY: (func $in-table | ||
| ;; PRIMARY-NEXT: (nop) | ||
| ;; PRIMARY-NEXT: ) | ||
| (func $in-table | ||
| ;; 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. | ||
|
Comment on lines
+89
to
+91
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does the code differentiate RefFuncs in a table from other RefFuncs?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, this was a part I meant to write Friday evening and somehow forgot 😄 Added now + testing. |
||
| ) | ||
|
|
||
| ;; SECONDARY: (func $second-in-table | ||
| ;; SECONDARY-NEXT: (nop) | ||
| ;; SECONDARY-NEXT: ) | ||
| (func $second-in-table | ||
| ;; As above, but in the secondary module. We still don't need a trampoline | ||
| ;; (but we will get a placeholder, as all split-out functions do). | ||
| ) | ||
| ) | ||
| ;; PRIMARY: (func $2 | ||
| ;; PRIMARY-NEXT: (call_indirect (type $0) | ||
| ;; PRIMARY-NEXT: (i32.const 2) | ||
| ;; PRIMARY-NEXT: ) | ||
| ;; PRIMARY-NEXT: ) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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