[GC] Ignore public types in GlobalTypeOptimization

[kripken]

TypeUpdater which it uses internally already does so, but we must also ignore
such types earlier, and make no other modifications to them.

Helps #7015

[kripken]

OTOH, SignaturePruning goes further than this:

binaryen/src/passes/SignaturePruning.cpp

Lines 294 to 297 in fd86ead

	// Rewrite the types. We pass in all the types we intend to modify as being
	// "additional private types" because we have proven above that they are
	// safe to modify, even if they are technically public (e.g. they may be in
	// a singleton big rec group that is public because one member is public).

It checks if individual types are exported etc., and not just their entire rec group.

That is more optimal, obviously, so I guess GTO should do the same? Or has our thinking changed since then @tlively ?

[kripken]

The SignaturePruning code would need work, as it errors on this (adapated from the same original testcase):

;; The struct type is public, so we cannot modify it and replace
;; the reference to the function type with the pruned version.
(module
  (rec
    (type $none (func))
    (type $much (func (param i32)))

    (type $struct (struct (field (ref $much))))
  )

  (func $exported (export "exported") (type $none)
    ;; This makes the rec group public.
  )

  (func $unused-param (type $much) (param $param i32)
    ;; We can remove this param.
  )

  (func $caller
    ;; We can remove this param.
    (call $unused-param
      (i32.const 0)
    )
  )

  (func $struct.new
    ;; This struct.new causes the problem mentioned above.
    (drop
      (struct.new $struct
        (ref.func $unused-param)
      )
    )
  )
)

[tlively]

We could recover some of the lost optimization potential by changing the field of child3 to be (ref nofunc) and continuing to drop the values that would be used to initialize it.

[tlively]

The extra optimizations SignaturePruning does are only safe because they leave the public types unchanged where they appear in the module interface and create new, optimized copies of the types for internal use. At least, that's the idea. I'm not sure it entirely makes sense, though. What if one of the exported functions is also referenced internally, and the code with the unoptimized type conflicts with the code with the optimized type?

[kripken]

We could recover some of the lost optimization potential by changing the field of child3 to be (ref nofunc) and continuing to drop the values that would be used to initialize it.

Sorry, I don't follow - what values would those be? That's an uninhabitable type so there is nothing we can use. I'm not sure how we can fix up this code, in other words:

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

It is not unreachable.

[tlively]

Oh, right, that wouldn't work because we need an inhabitable type. (ref null nofunc) would work if the original field were nullable, but otherwise I guess there's not much we can do.

[tlively]

I think we could do the same thing SignaturePruning does and create optimized copies of public struct types, but we could only do that safely in cases where the struct type is only public due to its rec group and not because it can actually flow out of the module. Right now we have no way to differentiate those different degrees of visibility.

[kripken]

@tlively

I think we could do the same thing SignaturePruning does and create optimized copies of public struct types, but we could only do that safely in cases where the struct type is only public due to its rec group and not because it can actually flow out of the module. Right now we have no way to differentiate those different degrees of visibility.

Yeah, that's exactly the issue. We'd need to identify those degrees, which means a very careful analysis of why something is public. I don't see a good way to do that atm, so I'll open a PR for SignaturePruning to limit it as well.

The good news is that at least on Java output I don't see any harm to limiting GTO and SignaturePruning in this way. But in theory on output like in #7015 (from Kotlin apparently) there is a single big rec group with mixed public/private contents that we could in theory partially optimize. But right now we break on them, so fixing things is a set up for them too.

[kripken]

Perhaps we can think of a way to split up public rec groups in closed world mode in a safe way, to make parts of such a group private again?

[tlively]

Perhaps we can think of a way to split up public rec groups in closed world mode in a safe way, to make parts of such a group private again?

See my comment here: #7015 (comment)

[tlively]

Yeah, that's exactly the issue. We'd need to identify those degrees, which means a very careful analysis of why something is public. I don't see a good way to do that atm, so I'll open a PR for SignaturePruning to limit it as well.

In principle we could do this by finding all the public types without considering rec group peers and giving them a different classification. But it would be better just to fix #6965 and let producers give us reasonable modules that don't have these issues to begin with.

[kripken]

Yeah, good point, #6965 would fix this too by having public annotations on types. That's better than on rec groups.