binaryen icon indicating copy to clipboard operation
binaryen copied to clipboard

GTO problem with shared rec groups

Open kripken opened this issue 1 year ago • 1 comments

(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 avatar Jun 04 '24 17:06 kripken

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 avatar Jun 04 '24 18:06 kripken

It seems this was fixed at some point.

tlively avatar Dec 13 '24 20:12 tlively