WebAssembly / binaryen

Optimizer and compiler/toolchain library for WebAssembly
Apache License 2.0
7.46k stars 740 forks source link

GTO problem with shared rec groups #6640

Open kripken opened 4 months ago

kripken commented 4 months ago
(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 commented 4 months ago

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...