-
Notifications
You must be signed in to change notification settings - Fork 814
DeadArgumentElimination: Refine result types of exported functions #7935
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
Conversation
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.
This is not safe in general because it changes the external type of a module, which can cause it to fail to link with other modules that it originally would have been able to link with. It does happen to work in the special case of exports being called by JS because JS does not care about the static types, but I don't think that's enough to allow us to do this by default.
The safe version of this optimization would be to give the exported functions new function types that are subtypes of their original types and have the refined result types. That would be kind of like doing TypeSSA on function types.
Right, thanks. But I guess we actually can't do this atm... func types are closed by default, so we can't subtype them. We'd need to emit open types in the fuzzer? (Last commit errors on the type being closed.) |
Ok, this should be correct now. |
// Exports prevent refining of argument types, as we don't see all the | ||
// calls. |
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.
And also refining the exported function type would allow generalizing the parameter types but not refining them.
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.
Yes (but this pass never generalizes).
;; CHECK: (type $func (sub (func (param anyref) (result anyref)))) | ||
(type $func (sub (func (param anyref) (result anyref)))) | ||
|
||
;; CHECK: (func $export (type $1) (param $x anyref) (result (ref any)) |
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.
It would be good to use --all-items in the auto update script so we can see the definition of type $1.
Before, we always made a new, simple (closed, and not a subtype of anything) type. Now we pick an interesting one sometimes. In particular, sometimes we'll get an open type here, allowing #7935 to do more (as it can only optimize DAE results on an open type).
Co-authored-by: Thomas Lively <tlively123@gmail.com>
Co-authored-by: Thomas Lively <tlively123@gmail.com>
Co-authored-by: Thomas Lively <tlively123@gmail.com>
@tlively The fuzzer found a bug: ;; bin/wasm-opt a.wat -all --dae --disable-custom-descriptors
;; We can refine the array result to $array. However, doing so brings in the
;; entire rec group of $array, which includes an exact type, which is not
;; allowed when custom descriptors are disabled.
(module
(type $func (sub (func (result (ref array)))))
(rec
(type $array (array i8))
(type $unused (func (result (ref (exact $array)))))
)
(func $test (export "test") (type $func) (result (ref array))
(array.new_fixed $array 0)
)
) Note how CD are disabled to show the bug. I actually don't know how to fix this. It seems like we would need to check if the refined type refers to a rec group that contains an exact type, and so on, recursively - whether it leads to any "bad" rec group becoming public..? iirc this is the type of issue we were worried about with the last-minute-lowering out of exactness when CD is disabled. Is there a good way to handle it? |
The reason we validate that exact types don't appear in the public interface is to make sure we don't accidentally erase exactness from a public type that the user actually intends to have an exact type. Since there is no user intention involved in this optimization, it's not really a problem to introduce a public exact type that will be erased later. Maybe we should remove that validation or find a way to run it only up front on the initial module. But the fact that this optimization can make private types public is actually very concerning. If we have all the types in a giant rec group, then we refine a public function type to return one of the private types, then every private type will become public and we won't be able to do any further type optimizations. That seems bad! Maybe we should enable this optimization only for fuzzing. |
Good point, it's really the private=>public issue here. I opened #7941 for that. (Hopefully the pass remains useful enough for fuzzing - should be, as types have a good chance to be public or private.) |
…tions" (#7943) Reverts #7935 Refining exports is unsafe given exact casts: the importing module can notice that we refined, as exact casts to the old type will fail. Doing this only when custom descriptors (exact casts) are disabled is possible, but rarely useful, and it's better to focus on optimizations that work with newer features.
Exports prevent most optimizations in this pass (we can't remove a
parameter, for example), but we can refine results: the export will just
return more-refined things, which is fine.
This also helps the fuzzer, in that it allows us to generate more
interesting testcases - we can modify the types of exports after the
fact.
Diff without whitespace is smaller.