Remove assertion about repeat types in TypeSSA #8176
Conversation
TypeSSA previously asserted that it never saw repeated type shapes when collecting the type shapes of the various types in the module. This is usually true, but it turns out that DAE can produce repeated type shapes that violated this assertion. Since it would be complicated to fix DAE to stop producing duplicate type shapes and the duplicate type shapes it produces are actually benign, simply remove the assertion in TypeSSA.
| @@ -0,0 +1,81 @@ | |||
| ;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. | |||
| ;; RUN: wasm-opt %s -all --disable-custom-descriptors --dae --type-ssa -S -o - | filecheck %s | |||
There was a problem hiding this comment.
Can't dae be run on this file in advance, and the testcase contain its results, and then only have type-ssa?
There was a problem hiding this comment.
The problem with trying to round-trip through text after DAE is that the conflicting rec groups are rejected by the parser after #8144, so the test would never get to running TypeSSA.
Maybe we should put more thought into trying to fix this in DAE to preserve the ability to do a text round trip after running it...
There was a problem hiding this comment.
Are you saying that DAE emits an invalid module? Or just invalid text?
There was a problem hiding this comment.
Only the text is invalid. The binary module will have the exact erased, so it will just have two definitions of the same type, but that's not a correctness or validation problem.
There was a problem hiding this comment.
We could consider generalizing types in the type printer the same way we do when we emit a binary, but I would be concerned about losing visibility into what's actually happening in the IR.
There was a problem hiding this comment.
FWIW I just found that --block-merging can produce text modules with the same issue. Sample input:
(module
(rec
(type $0 (func (result (ref $13) i32)))
(type $13 (struct))
(type $1 (sub (func (result (ref null struct) i32))))
)
(func $14 (type $1) (result (ref null struct) i32)
(drop
(block (result i32)
(i32.const 0)
)
)
(tuple.make 2
(struct.new_default $13)
(i32.const 0)
)
)
(func $15 (type $1) (result (ref null struct) i32)
(try_table (type $0) (result (ref $13) i32)
(tuple.make 2
(struct.new_default $13)
(i32.const 0)
)
)
)
)This builds my confidence that simply removing the assertion is the best move.
There was a problem hiding this comment.
Yes, I agree that seems best. Please add a comment on the new insertOrGet though, explaining why it is ok to reuse existing types, and the rest of the stuff in this thread.
TypeSSA previously asserted that it never saw repeated type shapes when
collecting the type shapes of the various types in the module. This is
usually true, but it turns out that DAE can produce repeated type shapes
that violated this assertion. Since it would be complicated to fix DAE
to stop producing duplicate type shapes and the duplicate type shapes it
produces are actually benign, simply remove the assertion in TypeSSA.