Skip to content

Remove assertion about repeat types in TypeSSA #8176

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tlively merged 2 commits into from
Jan 6, 2026
+85 −2
Merged

Remove assertion about repeat types in TypeSSA #8176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d3dd900
Remove assertion about repeat types in TypeSSA
tlively Jan 6, 2026
77de3dc
add comment
tlively Jan 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/passes/TypeSSA.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,10 @@ std::vector<HeapType> ensureTypesAreInNewRecGroup(std::vector<HeapType>&& types,
UniqueRecGroups unique(wasm.features);
for (auto group : existing) {
std::vector<HeapType> types(group.begin(), group.end());
[[maybe_unused]] auto uniqueTypes = unique.insert(std::move(types));
assert(uniqueTypes.size() == group.size() && "unexpected collision");
// N.B. we use `insertOrGet` rather than `insert` because some passes (DAE,
// BlockMerging) can create multiple types with the same shape, so we can't
// assume all the rec groups are already unique.
unique.insertOrGet(std::move(types));
}

auto num = types.size();
Expand Down
81 changes: 81 additions & 0 deletions test/lit/passes/dae-typessa-repeat-types.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member

@kripken kripken Jan 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't dae be run on this file in advance, and the testcase contain its results, and then only have type-ssa?

Copy link
Member Author

@tlively tlively Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

@kripken kripken Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying that DAE emits an invalid module? Or just invalid text?

Copy link
Member Author

@tlively tlively Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@tlively tlively Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@tlively tlively Jan 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@kripken kripken Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


(module
;; CHECK: (type $struct (struct))
(type $struct (struct))

;; CHECK: (type $1 (func (result i32 (ref (exact $struct)))))

;; CHECK: (type $array (sub (array (mut i32))))
(type $array (sub (array (mut i32))))

;; Trigger TypeSSA
;; CHECK: (type $3 (func))

;; CHECK: (type $array_1 (sub $array (array (mut i32))))

;; CHECK: (type $5 (func (result i32 (ref $struct))))

;; CHECK: (global $array (ref $array) (array.new $array_1
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: ))
(global $array (ref $array)
(array.new $array
(i32.const 0)
(i32.const 0)
)
)

;; CHECK: (func $caller (type $3)
;; CHECK-NEXT: (call $callee)
;; CHECK-NEXT: )
(func $caller
;; Give DAE a constant null parameter to optimize.
(tuple.drop 2
(call $callee
(ref.null none)
)
)
)

;; CHECK: (func $callee (type $3)
;; CHECK-NEXT: (local $0 anyref)
;; CHECK-NEXT: (tuple.drop 2
;; CHECK-NEXT: (block (type $1) (result i32 (ref (exact $struct)))
;; CHECK-NEXT: (local.set $0
;; CHECK-NEXT: (ref.null none)
;; CHECK-NEXT: )
;; CHECK-NEXT: (block (type $1) (result i32 (ref (exact $struct)))
;; CHECK-NEXT: (tuple.make 2
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: (struct.new_default $struct)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $callee (param (ref null any)) (result i32 (ref $struct))
;; When applying the constant null, DAE will create a block with
;; (result i32 (ref (exact $struct)))
(block (result i32 (ref $struct))
(tuple.make 2
(i32.const 0)
(struct.new_default $struct)
)
)
)

;; CHECK: (func $other (type $5) (result i32 (ref $struct))
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
(func $other (result i32 (ref $struct))
;; This will keep the (result i32 (ref $struct)) signature, which will
;; conflict with the (result i32 (ref (exact $struct))) of the block above
;; after binary writing. This will not be observable, though, since DAE only
;; optimizes non-referenced functions. TypeSSA should not crash or fail an
;; assertion due to the repeated type shape.
(unreachable)
)
)
Loading