Change the default type system to isorecursive

[tlively]

This makes Binaryen's default type system match the WasmGC spec.

Update the way type definitions without supertypes are printed to reduce the output diff for MVP tests that do not involve WasmGC. Also port some type-builder.cpp tests from test/example to test/gtest since they needed to be rewritten to work with isorecursive type anyway.

A follow-on PR will remove equirecursive types completely.

[kripken]

Should we finish fuzzing this mode before switching to it? Atm the fuzzer runs on --nominal and it looks like this PR doesn't change that.

[kripken]

Can we not print these if GC is not on? That is, even if the type system is isorecursive, we don't need these types if the features don't require it. It would also make the diff here much smaller I think.

[tlively]

Yeah, I added that logic and it helped some of the lit tests, but the problem is that most of these tests run with -all.

[tlively]

Current dependencies on/for this PR:

• main
  • PR Validate heap types #5279
    • PR Change the default type system to isorecursive #5239 👈
      • PR Remove equirecursive typing #5240

This comment was auto-generated by Graphite.

[tlively]

Should we finish fuzzing this mode before switching to it? Atm the fuzzer runs on --nominal and it looks like this PR doesn't change that.

Given that the old default was also not fuzzed, I wouldn't consider this a blocker.

[kripken]

We did at least fuzz equirecursive quite a lot before we switched the fuzzer over. It was fully stable back then. So we lost fuzz coverage but it had a good starting point at least. But, maybe it shouldn't block here. I am mainly concerned for any interactions with MVP wasm, but I guess such things should have been caught by tests.

[tlively]

Certainly fuzzing is a good idea. I can fuzz with isorecursive types overnight before landing this.

[kripken]

This diff is too large to comment on in the web UI 😄

What is this part of the diff?

diff --git a/test/example/type-builder.cpp b/test/example/type-builder.cpp
index 7da2f8f0383..2ce064b5ffe 100644
--- a/test/example/type-builder.cpp
+++ b/test/example/type-builder.cpp
@@ -18,26 +18,26 @@ void test_canonicalization() {
 
   TypeBuilder builder(4);
 
-  Type tempSigRef1 = builder.getTempRefType(builder[2], Nullable);
-  Type tempSigRef2 = builder.getTempRefType(builder[3], Nullable);
+  Type tempSigRef1 = builder.getTempRefType(builder[0], Nullable);
+  Type tempSigRef2 = builder.getTempRefType(builder[1], Nullable);

[tlively]

The order of types in that test needed to change so that the earlier types no longer referred to later types. That kind of forward reference is only allowed in recursion groups in the isorecursive system, but this test doesn't really need a recursion group, so I just changed the type order instead.

[kripken]

I had to write a python script to get through this massive diff 😄

lgtm, but please fuzz this even without changing the fuzzer to hybrid, as I think I saw an error. It was on a branch that I merged this PR to, but the error may have been due to the branch in theory... it did look like a type error though. I can't reproduce it outside of the branch but that might just be different random choices in the fuzzer.

Probably not helpful, but here is part of the log:

ITERATION: 1 seed: 17155922667202884028 size: 44650 (mean: 44650.0, stddev: 0.0) speed: 10255.022004889976 iters/sec,  0.0 wasm_bytes/sec
[..]
initial contents: /binaryen/test/./lit/recursive-types.wast
avoiding --flatten due to GC not supporting it (spilling of non-nullable locals)
avoiding --flatten due to GC not supporting it (spilling of non-nullable locals)
randomized opts: 
  -O3
  --roundtrip
  --simplify-locals-nonesting
  --monomorphize-always
  --generate-global-effects
  --monomorphize-always
  --rse
  -O1
  --generate-stack-ir
  --optimize-stack-ir
  -fimfs=99999999

/binaryen/bin/wasm-opt /binaryen/out/test/input.dat -ttf -o /binaryen/out/test/a.wasm --denan --legalize-js-interface --all-features --disable-threads --disable-mutable-globals --disable-simd --disable-bulk-memory --disable-tail-call --disable-gc --disable-memory64 --disable-relaxed-simd --disable-multi-memories --disable-memory64 --disable-multivalue --initial-fuzz=/binaryen/test/./lit/recursive-types.wast
warning: cannot de-nan outside of function context
warning: cannot de-nan outside of function context
warning: cannot de-nan outside of function context
warning: cannot de-nan outside of function context
warning: cannot de-nan outside of function context
warning: cannot de-nan outside of function context
pre wasm size: 16143
/binaryen/bin/wasm-opt a.wasm --all-features --disable-threads --disable-mutable-globals --disable-simd --disable-bulk-memory --disable-tail-call --disable-gc --disable-memory64 --disable-relaxed-simd --disable-multi-memories --disable-memory64 --disable-multivalue --print-features
[parse exception: Bad type form 2 (at 0:612)]
Fatal: error parsing wasm

(the branch was this PR + monomorphize)

[kripken]

Ok, I got an error with just this PR. Seed: 11374867808572504832, using --auto-initial-contents. Reducing it now.

[kripken]

Oh, it could actually be array.new_data, perhaps related to #5236 . Probably best to fix that first.

[tlively]

This should be ready to land now soon. I'm doing final fuzzing now.

[tlively]

Ran over 7k fuzzer iterations at this latest commit with TYPE_SYSTEM_FLAG set to --hybrid.

[kripken]

I can take another look now. I see force-pushes though, so it's a little hard to see what's changed since I read this last. Is there a way to do that on github?

About fuzzing, if we aren't also changing the default in fuzz_opt to the new mode, then I'd also fuzz the default in that file. I also recommend fuzzing for a larger amount, for a change like this - I often find bugs only after 50K or 100K.

[tlively]

Sounds good, will fuzz more both with and without the flag changed. Sorry about the force-pushes, I know they make review more difficult. That's the trade off I'm making to get to use stacked PRs. I would welcome feedback about whether that's a good trade off or not on the review side.

I'm not squashing or changing any existing commits during those force pushes (except that merge conflicts get resolved in whatever arbitrary commit the conflict occurred at), so you can always look at the individual new commits to see what has changed. GitHub doesn't have a good way of seeing how many commits are new since your last review in the presence of force pushes, though.

[kripken]

Interesting, thanks, if the commit contents don't change then I could in theory remember by their names which I've reviewed...

[kripken]

Ok, pretty sure I've reviewed the first one before, and the others look ok together, so lgtm with that fuzzing.

[tlively]

Fuzzed just under 50k iterations on this before merging.