Avoid duplicate type names

[kripken]

The patch is simple but the large diff in tests is quite annoying. I think I figured out at least part of the problem but I don't have a good idea how to fix it:

$ bin/wasm-opt a.wat -all --print --unsubtyping --print --remove-unused-types --print --closed-world (module (type $top (sub (struct ))) (type $1 (func)) (type $mid (sub $top (struct ))) (type $bot (sub $mid (struct ))) (func $cast (type $1) (local $l (ref null $top)) (local.set $l (struct.new_default $bot) ) (drop (ref.cast (ref $mid) (struct.new_default $top) ) ) ) ) (module (rec (type $top (sub (struct ))) (type $mid (sub $top (struct ))) (type $bot (sub $mid (struct ))) (type $3 (func)) ) (func $cast (type $3) (local $l (ref null $top)) (local.set $l (struct.new_default $bot) ) (drop (ref.cast (ref $mid) (struct.new_default $top) ) ) ) ) (module (rec (type $top_1 (sub (struct ))) (type $mid_1 (sub $top_1 (struct ))) (type $bot_1 (sub $mid_1 (struct ))) (type $3 (func)) ) (func $cast (type $3) (local $l (ref null $top_1)) (local.set $l (struct.new_default $bot_1) ) (drop (ref.cast (ref $mid_1) (struct.new_default $top_1) ) ) ) )

IIANM what happens is this:

• --print shows the original module. Note each type is in its own rec group.
• --unsubtyping --print creates a new singleton rec group for them. Note how they still have the proper names, because the patch makes the new types have the old names, and modifies the old names, but luckily the old names are no longer used.
• --remove-unused-types --print looks identical except that now the names are have ugly _1 suffixes. That pass refreshes the private types by rebuilding them. It turns out that the new rec group we create is identical to the old, so they get folded together. In particular it seems that the new rec group, with the good names, is folded into the old rec group, with the ugly names.

[tlively]

I think we can just skip adding new names entirely, including the deduping logic, when old == new.

[kripken]

Thanks! Makes sense, I guess it is simple to just check if the types changed or not. I added that now.

This almost removes the entire diff, but (aside from a correct diff to a signature-pruning test) there is still a diff on an unsubtyping test. The test is this:

(module (type $super (sub (struct))) (type $sub (sub $super (struct))) (tag $t (param (ref $super))) (func $throw (throw $t (struct.new $sub) ) ) )

Roundtripping that text leads to

(module (type $super (sub (struct ))) (type $1 (func (param (ref $super)))) (type $2 (func)) (type $sub (sub $super (struct ))) (tag $t (param (ref $super))) (func $throw (type $2) (throw $t (struct.new_default $sub) ) ) )

and that seemingly-unused (no references in the text format) type $1 seems to be the problem later, as after unsubtyping we have

(module (rec (type $super (sub (struct ))) (type $sub (sub $super (struct ))) (type $2 (func)) (type $3 (func (param (ref $super)))) ) (type $4 (func (param (ref $super)))) (tag $t (param (ref $super))) (func $throw (type $2) (throw $t (struct.new_default $sub) ) ) )

and after remove-unused-types we have

(module (rec (type $super_1 (sub (struct ))) (type $sub_1 (sub $super_1 (struct ))) (type $2 (func)) (type $3 (func (param (ref $super_1)))) ) (type $4 (func (param (ref $super_1)))) (tag $t (param (ref $super_1))) (func $throw (type $2) (throw $t (struct.new_default $sub_1) ) ) )

Note $3 and $4.

@tlively is there a reason we don't have a HeapType in Tags and just a Signature? I am guessing that we create the type implicitly and that is related here.

[kripken]

To be clear, the issue is that the final comparison ends up comparing the type from outside the rec group to one inside it, so they are unequal, and since types changed it updates the names which end up uglier.

I didn't dig into why it sees the old type as the one from outside the rec group but I'm guessing it's due to not having a HeapType on Tags - if that doesn't sound right though I can investigate more.

[tlively]

Yeah, the binary writer will implicitly create a function heap type for the signature on the tag if there is not already a type with a matching signature. This implicitly created type will not be in a rec group with any other type. We should update tags to use heap types internally.

[tlively]

If you want to be fancy about this ;)

Suggested change

	auto newType = newTypes[index];
	oldToNewTypes[type] = newType;
	if (newType != type) {
	if (type != (oldToNewTypes[type] = newTypes[index])) {

[kripken]

Moot after the last commit, but that would definitely have been more concise.

[kripken]

Got it, thanks. Yeah, that seems important to do, as right now we are actually shipping an unneeded type in these cases (we add tag types to the singleton rec group).

But I realized we can work around it here in a reasonable way but just comparing on a type by type basis rather than the entire rec group, that seems to work, PTAL.