From 639b34ffea9ad7c9c640a1a503c79f6973512279 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 21 Oct 2024 09:09:45 -0700 Subject: [PATCH 1/3] fix --- src/passes/GlobalTypeOptimization.cpp | 23 +++++++++++++++++++---- test/lit/passes/gto-removals.wast | 24 ++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/src/passes/GlobalTypeOptimization.cpp b/src/passes/GlobalTypeOptimization.cpp index 74107f9cdd3..a6163aa52cd 100644 --- a/src/passes/GlobalTypeOptimization.cpp +++ b/src/passes/GlobalTypeOptimization.cpp @@ -314,12 +314,27 @@ struct GlobalTypeOptimization : public Pass { // 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 + // propagate to both), except for the corner case of a 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))); +#ifndef NDEBUG + if (removableIndexes.count(i)) { + // We want to remove it, so as as just mentioned, a super must + // be public while we are private, or else |superIndexes| has + // invalid data. + assert(!publicTypesSet.count(type)); + auto foundPublicSuper = false; + auto currSuper = super; + while (currSuper) { + if (publicTypesSet.count(*currSuper)) { + foundPublicSuper = true; + break; + } + currSuper = currSuper->getDeclaredSuperType(); + } + assert(foundPublicSuper); + } +#endif // 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 diff --git a/test/lit/passes/gto-removals.wast b/test/lit/passes/gto-removals.wast index 61396cf8f7a..3745b03f32a 100644 --- a/test/lit/passes/gto-removals.wast +++ b/test/lit/passes/gto-removals.wast @@ -1495,3 +1495,27 @@ ) ) ) + +;; 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))))) + ;; 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 $C + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: )) + (global $global (ref $A) (struct.new $C + (i32.const 0) + )) + + ;; CHECK: (export "global" (global $global)) + (export "global" (global $global)) +) + From 18c0dbd6781b767f9212e495c9462744b8ae142d Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 21 Oct 2024 12:12:03 -0700 Subject: [PATCH 2/3] fix --- src/passes/GlobalTypeOptimization.cpp | 28 +++-------------- test/lit/passes/gto-removals.wast | 44 +++++++++++++++++++++++---- 2 files changed, 42 insertions(+), 30 deletions(-) diff --git a/src/passes/GlobalTypeOptimization.cpp b/src/passes/GlobalTypeOptimization.cpp index a6163aa52cd..e35e30ac8da 100644 --- a/src/passes/GlobalTypeOptimization.cpp +++ b/src/passes/GlobalTypeOptimization.cpp @@ -311,30 +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 a parent - // being public but us being private (the propagation does not - // take into account visibility). -#ifndef NDEBUG - if (removableIndexes.count(i)) { - // We want to remove it, so as as just mentioned, a super must - // be public while we are private, or else |superIndexes| has - // invalid data. - assert(!publicTypesSet.count(type)); - auto foundPublicSuper = false; - auto currSuper = super; - while (currSuper) { - if (publicTypesSet.count(*currSuper)) { - foundPublicSuper = true; - break; - } - currSuper = currSuper->getDeclaredSuperType(); - } - assert(foundPublicSuper); - } -#endif + // 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 diff --git a/test/lit/passes/gto-removals.wast b/test/lit/passes/gto-removals.wast index 3745b03f32a..a445ed4a918 100644 --- a/test/lit/passes/gto-removals.wast +++ b/test/lit/passes/gto-removals.wast @@ -1508,12 +1508,44 @@ ;; Use $C so it isn't removed trivially, which also keeps $B alive as its ;; super. - ;; CHECK: (global $global (ref $A) (struct.new $C - ;; CHECK-NEXT: (i32.const 0) - ;; CHECK-NEXT: )) - (global $global (ref $A) (struct.new $C - (i32.const 0) - )) + ;; 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, so it is on the immediate +;; parent, preventing removal in $C. +(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)) From 8f9f08b8aa703587cb0e61795e74f896d1bff29b Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 21 Oct 2024 12:14:45 -0700 Subject: [PATCH 3/3] tests --- test/lit/passes/gto-removals.wast | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/test/lit/passes/gto-removals.wast b/test/lit/passes/gto-removals.wast index a445ed4a918..99579f8ab1d 100644 --- a/test/lit/passes/gto-removals.wast +++ b/test/lit/passes/gto-removals.wast @@ -1533,14 +1533,13 @@ (export "global" (global $global)) ) -;; As above, but the f64 field is now on $B as well, so it is on the immediate -;; parent, preventing removal in $C. +;; 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))))) + (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))))) @@ -1551,3 +1550,25 @@ (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)) +) +