Skip to content
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

GTO problem with shared rec groups #6640

Open
kripken opened this issue Jun 4, 2024 · 1 comment
Open

GTO problem with shared rec groups #6640

kripken opened this issue Jun 4, 2024 · 1 comment

Comments

@kripken
Copy link
Member

kripken commented Jun 4, 2024

(module
 (rec
  (type $import (func))
  (type $struct (sub (struct (field (ref func)))))
 )

 (import "imports" "import" (func $fimport (type $import)))

 (global $global (ref $struct) (struct.new $struct
  (ref.func $func)
 ))

 (func $func
  (unreachable)
 )
)
$ bin/wasm-opt y.wat -all --gto --closed-world
[wasm-validator error in module] unexpected false: struct.new_with_default value type must be defaultable, on 
(ref func)
Fatal: error after opts

This is related to #6630. What happens here is that the import causes the entire rec group to be public. Then GTO decides it can remove the field from the struct, but TypeUpdating ignores public structs when it does that rewriting, so we end up not modifying that type, and the module is broken.

As in #6630 we could inform TypeUpdating that the types we modify are modifiable despite their rec group being public. However, as this is the second time we see this I am starting to think this is a general pattern.

What I suspect might be happening is that in J2Wasm output the imports begin without having their types as part of the big rec group. But at some point during optimization they end up there, turning everything else public. That might be worth looking into. But while it would work around this, notifying TypeUpdating of GTO types may still make sense because it is otherwise a missing optimization opportunity.

@kripken
Copy link
Member Author

kripken commented Jun 4, 2024

I verified the original wat from J2Wasm does not declare a type for that import ("RegExp.test$1"), so the type is generated implicitly and has its own rec group. Roundtripping does not affect that. RemoveUnusedTypes does not break that either, nor do other optimization passes, but I see that StringLowering does...

kripken added a commit that referenced this issue Jun 10, 2024
…6642)

We need StringLowering to modify even public types, as it must replace every
single stringref with externref, even if that modifies the ABI. To achieve that
we told it that all string-using types were private, which let TypeUpdater update
them, but the problem is that it moves all private types to a new single
rec group, which meant public and private types ended up in the same group.
As a result, a single public type would make it all public, preventing optimizations
and breaking things as in #6630 #6640.

Ideally TypeUpdater would modify public types while keeping them in the same
rec groups, but this may be a very specific issue for StringLowering, and that
might be a lot of work. Instead, just make StringLowering handle public types of
functions in a manual way, which is simple and should handle all cases that
matter in practice, at least in J2Wasm.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant