-
Notifications
You must be signed in to change notification settings - Fork 839
Intrinsic: js.called #8324
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
base: main
Are you sure you want to change the base?
Intrinsic: js.called #8324
Changes from all commits
f593169
85d72fc
86647de
78e19f0
922b786
25524f7
c6ffea0
c65b6bd
9de37fc
27d8861
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 |
|---|---|---|
|
|
@@ -176,9 +176,8 @@ struct SignaturePruning : public Pass { | |
| allInfo[tag->type].optimizable = false; | ||
| } | ||
|
|
||
| // configureAll functions are signature-called, and must also not be | ||
| // modified. | ||
| for (auto func : Intrinsics(*module).getConfigureAllFunctions()) { | ||
| // Signature-called functions must also not be modified. | ||
|
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. In principle we could still remove a suffix of the parameters, although I'm not sure whether that comes with a performance overhead. Maybe worth checking on that and adding a TODO if there would be no downside. |
||
| for (auto func : Intrinsics(*module).getJSCalledFunctions()) { | ||
| allInfo[module->getFunction(func)->type.getHeapType()].optimizable = | ||
| false; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -162,9 +162,8 @@ struct SignatureRefining : public Pass { | |
| } | ||
| } | ||
|
|
||
| // configureAll functions are signature-called, which means their params | ||
| // must not be refined. | ||
| for (auto func : Intrinsics(*module).getConfigureAllFunctions()) { | ||
| // Signature-called functions must not have params refined. | ||
|
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. Would it make sense to retire the "signature-called" terminology? |
||
| for (auto func : Intrinsics(*module).getJSCalledFunctions()) { | ||
| allInfo[module->getFunction(func)->type.getHeapType()].canModifyParams = | ||
| false; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| ;; RUN: wasm-opt -all %s -S -o - | filecheck %s | ||
|
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. This can use the update script now. |
||
| ;; RUN: wasm-opt -all --roundtrip %s -S -o - | filecheck %s | ||
|
|
||
| ;; Test text and binary handling of @binaryen.js.called. | ||
|
|
||
| (module | ||
| (@binaryen.js.called) | ||
| (func $func-annotation | ||
| (drop | ||
| (i32.const 0) | ||
| ) | ||
| ) | ||
| ) | ||
|
|
||
| ;; CHECK: (module | ||
| ;; CHECK-NEXT: (type $0 (func)) | ||
| ;; CHECK-NEXT: (@binaryen.js.called) | ||
| ;; CHECK-NEXT: (func $func-annotation | ||
| ;; CHECK-NEXT: (drop | ||
| ;; CHECK-NEXT: (i32.const 0) | ||
| ;; CHECK-NEXT: ) | ||
| ;; CHECK-NEXT: ) | ||
| ;; CHECK-NEXT: ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| ;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. | ||
|
|
||
| ;; RUN: foreach %s %t wasm-opt --remove-unused-module-elements --closed-world -all -S -o - | filecheck %s | ||
|
|
||
| (module | ||
| (@binaryen.js.called) | ||
|
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. I think the latest changes to the update script should put this annotation down with the function. |
||
| ;; CHECK: (type $0 (func)) | ||
|
|
||
| ;; CHECK: (elem declare func $js.called.referred) | ||
|
|
||
| ;; CHECK: (export "export" (func $export)) | ||
|
|
||
| ;; CHECK: (func $js.called.referred (type $0) | ||
| ;; CHECK-NEXT: (drop | ||
| ;; CHECK-NEXT: (i32.const 10) | ||
| ;; CHECK-NEXT: ) | ||
| ;; CHECK-NEXT: ) | ||
| (func $js.called.referred | ||
| ;; This is jsCalled, and referred below, so it is kept. | ||
| (drop (i32.const 10)) | ||
| ) | ||
|
|
||
| ;; CHECK: (func $export (type $0) | ||
| ;; CHECK-NEXT: (drop | ||
| ;; CHECK-NEXT: (ref.func $js.called.referred) | ||
| ;; CHECK-NEXT: ) | ||
| ;; CHECK-NEXT: ) | ||
| (func $export (export "export") | ||
| (drop (ref.func $js.called.referred)) | ||
| ) | ||
|
|
||
| (@binaryen.js.called) | ||
| (func $js.called.unreferred | ||
| ;; This is jsCalled, and not referred anywhere. The annotation does not | ||
| ;; stop the function from being removed entirely. | ||
| (drop (i32.const 20)) | ||
| ) | ||
| ) | ||
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.
Are we still planning to eventually phase out this special handling for configureAll? I think we should to make the usage more consistent across different configureAll usage patterns.