Skip to content

[GC] Fix assertion in GlobalTypeOptimization about public super #7026

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
merged 3 commits into from
Oct 22, 2024
Merged

[GC] Fix assertion in GlobalTypeOptimization about public super #7026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
639b34f
fix
kripken Oct 21, 2024
18c0dbd
fix
kripken Oct 21, 2024
8f9f08b
tests
kripken Oct 21, 2024
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
13 changes: 4 additions & 9 deletions src/passes/GlobalTypeOptimization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,15 +311,10 @@ struct GlobalTypeOptimization : public Pass {
keptFieldsNotInSuper.push_back(i);
}
} else {
// The super kept this field, so we must keep it as well. The
// propagation analysis above ensures that we and the super are in
// agreement on keeping it (the reasons that prevent optimization
// propagate to both), except for the corner case of the parent
// being public but us being private (the propagation does not
// take into account visibility).
assert(
!removableIndexes.count(i) ||
(publicTypesSet.count(*super) && !publicTypesSet.count(type)));
// The super kept this field, so we must keep it as well. This can
// happen when we need the field in both, but also in the corner
// case where we don't need the field but the super is public.

// We need to keep it at the same index so we remain compatible.
indexesAfterRemoval[i] = superIndex;
// Update |next| to refer to the next available index. Due to
Expand Down
77 changes: 77 additions & 0 deletions test/lit/passes/gto-removals.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1495,3 +1495,80 @@
)
)
)

;; The type $A is public because it is on an exported global. As a result we
;; cannot remove the unused i32 field from its child or grandchild.
(module
;; CHECK: (type $A (sub (struct (field (mut i32)))))
(type $A (sub (struct (field (mut i32)))))
Copy link
Member

@tlively tlively Oct 21, 2024

Choose a reason for hiding this comment

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

Should we also add a test where the field is not present on $A, so it can still be removed from $B and $C?

Copy link
Member Author

@kripken kripken Oct 21, 2024

Choose a reason for hiding this comment

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

Good catch, this was actually a bug. The loop on supers here would need to also handle the super not having the field, mirroring the code elsewhere.

Overall this seems to make the assertion more and more complex, past the point of being worth it, so I removed it. We do still cover this in depth in tests.

;; CHECK: (type $B (sub $A (struct (field (mut i32)))))
(type $B (sub $A (struct (field (mut i32)))))
;; CHECK: (type $C (sub $B (struct (field (mut i32)))))
(type $C (sub $B (struct (field (mut i32)))))

;; Use $C so it isn't removed trivially, which also keeps $B alive as its
;; super.
;; CHECK: (global $global (ref $A) (struct.new_default $C))
(global $global (ref $A) (struct.new_default $C))

;; CHECK: (export "global" (global $global))
(export "global" (global $global))
)

;; As above, but now there is an f64 field on $C that can be removed, since it
;; is not on the parents.
(module
;; CHECK: (type $A (sub (struct (field (mut i32)))))
(type $A (sub (struct (field (mut i32)))))
;; CHECK: (rec
;; CHECK-NEXT: (type $B (sub $A (struct (field (mut i32)))))
(type $B (sub $A (struct (field (mut i32)))))
;; CHECK: (type $C (sub $B (struct (field (mut i32)))))
(type $C (sub $B (struct (field (mut i32)) (field (mut f64)))))

;; CHECK: (global $global (ref $A) (struct.new_default $C))
(global $global (ref $A) (struct.new_default $C))

;; CHECK: (export "global" (global $global))
(export "global" (global $global))
)

;; As above, but the f64 field is now on $B as well. We can still remove it.
(module
;; CHECK: (type $A (sub (struct (field (mut i32)))))
(type $A (sub (struct (field (mut i32)))))
;; CHECK: (rec
;; CHECK-NEXT: (type $B (sub $A (struct (field (mut i32)))))
(type $B (sub $A (struct (field (mut i32)) (field (mut f64)))))
;; CHECK: (type $C (sub $B (struct (field (mut i32)))))
(type $C (sub $B (struct (field (mut i32)) (field (mut f64)))))

;; CHECK: (global $global (ref $A) (struct.new_default $C))
(global $global (ref $A) (struct.new_default $C))

;; CHECK: (export "global" (global $global))
(export "global" (global $global))
)

;; As above, but now $B is public as well. Now we cannot remove the f64.
(module
;; CHECK: (type $A (sub (struct (field (mut i32)))))
(type $A (sub (struct (field (mut i32)))))
;; CHECK: (type $B (sub $A (struct (field (mut i32)) (field (mut f64)))))
(type $B (sub $A (struct (field (mut i32)) (field (mut f64)))))
;; CHECK: (type $C (sub $B (struct (field (mut i32)) (field (mut f64)))))
(type $C (sub $B (struct (field (mut i32)) (field (mut f64)))))

;; CHECK: (global $global (ref $A) (struct.new_default $C))
(global $global (ref $A) (struct.new_default $C))

;; CHECK: (global $globalB (ref $B) (struct.new_default $C))
(global $globalB (ref $B) (struct.new_default $C))

;; CHECK: (export "global" (global $global))
(export "global" (global $global))

;; CHECK: (export "globalB" (global $globalB))
(export "globalB" (global $globalB))
)

Loading