Skip to content

Commit d71161a

Browse files
kripkenkripken
authored
[GC] Fix GlobalTypeOptimization logic for public types handling (#7051)
This fixes a regression from #7019. That PR fixed an error on situations with mixed public and private types, but it made us stop optimizing in valid cases, including cases with entirely private types. The specific regression was that we checked if we had an entry in the map of "can become immutable", and we thought that was enough. But we may have a private child type with a public parent, and still be able to optimize in the child if the field is not present in the parent. We also did not have exhaustive checking of all the states canBecomeImmutable can be, so add those + testing.
1 parent c35e987 commit d71161a

File tree

2 files changed

+342
-7
lines changed

2 files changed

+342
-7
lines changed

‎src/passes/GlobalTypeOptimization.cpp

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -203,13 +203,24 @@ struct GlobalTypeOptimization : public Pass {
203203
// visibility, so do that here: we can only become immutable if the
204204
// parent can as well.
205205
auto super = type.getDeclaredSuperType();
206-
if (super && !canBecomeImmutable.count(*super)) {
207-
// No entry in canBecomeImmutable means nothing in the parent can
208-
// become immutable. We don't need to check the specific field index,
209-
// because visibility affects them all equally (i.e., if it is public
210-
// then no field can be changed, and if it is private then this field
211-
// can be changed, and perhaps more).
212-
continue;
206+
if (super) {
207+
// The super may not contain the field, which is fine, so only check
208+
// here if the field does exist in both.
209+
if (i < super->getStruct().fields.size()) {
210+
// No entry in canBecomeImmutable means nothing in the parent can
211+
// become immutable, so check for both that and for an entry with
212+
// "false".
213+
auto iter = canBecomeImmutable.find(*super);
214+
if (iter == canBecomeImmutable.end()) {
215+
continue;
216+
}
217+
// The vector is grown only when needed to contain a "true" value,
218+
// so |i| being out of bounds indicates "false".
219+
auto& superVec = iter->second;
220+
if (i >= superVec.size() || !superVec[i]) {
221+
continue;
222+
}
223+
}
213224
}
214225

215226
// No set exists. Mark it as something we can make immutable.

‎test/lit/passes/gto-mutability.wast

Lines changed: 324 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -688,3 +688,327 @@
688688
)
689689
)
690690

691+
;; $sub has a field we can make immutable. That it does not exist in the super
692+
;; should not confuse us.
693+
(module
694+
(rec
695+
;; CHECK: (rec
696+
;; CHECK-NEXT: (type $super (sub (struct)))
697+
(type $super (sub (struct)))
698+
;; CHECK: (type $sub (sub $super (struct (field (ref string)))))
699+
(type $sub (sub $super (struct (field (mut (ref string))))))
700+
)
701+
702+
;; CHECK: (type $2 (func))
703+
704+
;; CHECK: (func $test (type $2)
705+
;; CHECK-NEXT: (drop
706+
;; CHECK-NEXT: (struct.get $sub 0
707+
;; CHECK-NEXT: (struct.new $sub
708+
;; CHECK-NEXT: (string.const "foo")
709+
;; CHECK-NEXT: )
710+
;; CHECK-NEXT: )
711+
;; CHECK-NEXT: )
712+
;; CHECK-NEXT: )
713+
(func $test
714+
;; Write and read the field.
715+
(drop
716+
(struct.get $sub 0
717+
(struct.new $sub
718+
(string.const "foo")
719+
)
720+
)
721+
)
722+
)
723+
)
724+
725+
;; As above, but with another type in the middle, $mid, which also contains the
726+
;; field. We can optimize both $mid and $sub.
727+
(module
728+
(rec
729+
;; CHECK: (rec
730+
;; CHECK-NEXT: (type $super (sub (struct)))
731+
(type $super (sub (struct)))
732+
;; CHECK: (type $mid (sub $super (struct (field (ref string)))))
733+
(type $mid (sub $super (struct (field (mut (ref string))))))
734+
;; CHECK: (type $sub (sub $mid (struct (field (ref string)))))
735+
(type $sub (sub $mid (struct (field (mut (ref string))))))
736+
)
737+
738+
;; CHECK: (type $3 (func))
739+
740+
;; CHECK: (func $test (type $3)
741+
;; CHECK-NEXT: (drop
742+
;; CHECK-NEXT: (struct.get $sub 0
743+
;; CHECK-NEXT: (struct.new $sub
744+
;; CHECK-NEXT: (string.const "foo")
745+
;; CHECK-NEXT: )
746+
;; CHECK-NEXT: )
747+
;; CHECK-NEXT: )
748+
;; CHECK-NEXT: (drop
749+
;; CHECK-NEXT: (struct.get $mid 0
750+
;; CHECK-NEXT: (struct.new $mid
751+
;; CHECK-NEXT: (string.const "bar")
752+
;; CHECK-NEXT: )
753+
;; CHECK-NEXT: )
754+
;; CHECK-NEXT: )
755+
;; CHECK-NEXT: )
756+
(func $test
757+
(drop
758+
(struct.get $sub 0
759+
(struct.new $sub
760+
(string.const "foo")
761+
)
762+
)
763+
)
764+
(drop
765+
(struct.get $mid 0
766+
(struct.new $mid
767+
(string.const "bar")
768+
)
769+
)
770+
)
771+
)
772+
)
773+
774+
;; As above, but add another irrelevant field first. We can still optimize the
775+
;; string, but the new mutable i32 must remain mutable, as it has a set.
776+
(module
777+
(rec
778+
;; CHECK: (rec
779+
;; CHECK-NEXT: (type $super (sub (struct (field (mut i32)))))
780+
(type $super (sub (struct (field (mut i32)))))
781+
;; CHECK: (type $mid (sub $super (struct (field (mut i32)) (field (ref string)))))
782+
(type $mid (sub $super (struct (field (mut i32)) (field (mut (ref string))))))
783+
;; CHECK: (type $sub (sub $mid (struct (field (mut i32)) (field (ref string)))))
784+
(type $sub (sub $mid (struct (field (mut i32)) (field (mut (ref string))))))
785+
)
786+
787+
;; CHECK: (type $3 (func))
788+
789+
;; CHECK: (func $test (type $3)
790+
;; CHECK-NEXT: (drop
791+
;; CHECK-NEXT: (struct.get $sub 1
792+
;; CHECK-NEXT: (struct.new $sub
793+
;; CHECK-NEXT: (i32.const 42)
794+
;; CHECK-NEXT: (string.const "foo")
795+
;; CHECK-NEXT: )
796+
;; CHECK-NEXT: )
797+
;; CHECK-NEXT: )
798+
;; CHECK-NEXT: (drop
799+
;; CHECK-NEXT: (struct.get $mid 1
800+
;; CHECK-NEXT: (struct.new $mid
801+
;; CHECK-NEXT: (i32.const 1337)
802+
;; CHECK-NEXT: (string.const "bar")
803+
;; CHECK-NEXT: )
804+
;; CHECK-NEXT: )
805+
;; CHECK-NEXT: )
806+
;; CHECK-NEXT: (struct.set $super 0
807+
;; CHECK-NEXT: (struct.new $super
808+
;; CHECK-NEXT: (i32.const 98765)
809+
;; CHECK-NEXT: )
810+
;; CHECK-NEXT: (i32.const 42)
811+
;; CHECK-NEXT: )
812+
;; CHECK-NEXT: (drop
813+
;; CHECK-NEXT: (struct.get $super 0
814+
;; CHECK-NEXT: (struct.new $super
815+
;; CHECK-NEXT: (i32.const 999999)
816+
;; CHECK-NEXT: )
817+
;; CHECK-NEXT: )
818+
;; CHECK-NEXT: )
819+
;; CHECK-NEXT: )
820+
(func $test
821+
(drop
822+
(struct.get $sub 1
823+
(struct.new $sub
824+
(i32.const 42)
825+
(string.const "foo")
826+
)
827+
)
828+
)
829+
(drop
830+
(struct.get $mid 1
831+
(struct.new $mid
832+
(i32.const 1337)
833+
(string.const "bar")
834+
)
835+
)
836+
)
837+
;; A set and get of the first field.
838+
(struct.set $super 0
839+
(struct.new $super
840+
(i32.const 98765)
841+
)
842+
(i32.const 42)
843+
)
844+
(drop
845+
(struct.get $super 0
846+
(struct.new $super
847+
(i32.const 999999)
848+
)
849+
)
850+
)
851+
)
852+
)
853+
854+
;; As above, but without a set of the first field. Now we can optimize both
855+
;; fields.
856+
(module
857+
(rec
858+
;; CHECK: (rec
859+
;; CHECK-NEXT: (type $super (sub (struct (field i32))))
860+
(type $super (sub (struct (field (mut i32)))))
861+
;; CHECK: (type $mid (sub $super (struct (field i32) (field (ref string)))))
862+
(type $mid (sub $super (struct (field (mut i32)) (field (mut (ref string))))))
863+
;; CHECK: (type $sub (sub $mid (struct (field i32) (field (ref string)))))
864+
(type $sub (sub $mid (struct (field (mut i32)) (field (mut (ref string))))))
865+
)
866+
867+
;; CHECK: (type $3 (func))
868+
869+
;; CHECK: (func $test (type $3)
870+
;; CHECK-NEXT: (drop
871+
;; CHECK-NEXT: (struct.get $sub 1
872+
;; CHECK-NEXT: (struct.new $sub
873+
;; CHECK-NEXT: (i32.const 42)
874+
;; CHECK-NEXT: (string.const "foo")
875+
;; CHECK-NEXT: )
876+
;; CHECK-NEXT: )
877+
;; CHECK-NEXT: )
878+
;; CHECK-NEXT: (drop
879+
;; CHECK-NEXT: (struct.get $mid 1
880+
;; CHECK-NEXT: (struct.new $mid
881+
;; CHECK-NEXT: (i32.const 1337)
882+
;; CHECK-NEXT: (string.const "bar")
883+
;; CHECK-NEXT: )
884+
;; CHECK-NEXT: )
885+
;; CHECK-NEXT: )
886+
;; CHECK-NEXT: (drop
887+
;; CHECK-NEXT: (struct.get $super 0
888+
;; CHECK-NEXT: (struct.new $super
889+
;; CHECK-NEXT: (i32.const 999999)
890+
;; CHECK-NEXT: )
891+
;; CHECK-NEXT: )
892+
;; CHECK-NEXT: )
893+
;; CHECK-NEXT: )
894+
(func $test
895+
(drop
896+
(struct.get $sub 1
897+
(struct.new $sub
898+
(i32.const 42)
899+
(string.const "foo")
900+
)
901+
)
902+
)
903+
(drop
904+
(struct.get $mid 1
905+
(struct.new $mid
906+
(i32.const 1337)
907+
(string.const "bar")
908+
)
909+
)
910+
)
911+
;; Only a get of the first field.
912+
(drop
913+
(struct.get $super 0
914+
(struct.new $super
915+
(i32.const 999999)
916+
)
917+
)
918+
)
919+
)
920+
)
921+
922+
;; The super is public, but we can still optimize the field in the sub.
923+
(module
924+
;; CHECK: (type $super (sub (struct)))
925+
(type $super (sub (struct)))
926+
927+
;; CHECK: (rec
928+
;; CHECK-NEXT: (type $sub (sub $super (struct (field stringref))))
929+
(type $sub (sub $super (struct (field (mut stringref)))))
930+
931+
;; CHECK: (type $2 (func))
932+
933+
;; CHECK: (global $global (ref $super) (struct.new_default $super))
934+
(global $global (ref $super) (struct.new_default $super))
935+
;; CHECK: (export "global" (global $global))
936+
(export "global" (global $global))
937+
938+
;; CHECK: (func $test (type $2)
939+
;; CHECK-NEXT: (drop
940+
;; CHECK-NEXT: (struct.get $sub 0
941+
;; CHECK-NEXT: (struct.new $sub
942+
;; CHECK-NEXT: (string.const "foo")
943+
;; CHECK-NEXT: )
944+
;; CHECK-NEXT: )
945+
;; CHECK-NEXT: )
946+
;; CHECK-NEXT: )
947+
(func $test
948+
;; Write and read the field.
949+
(drop
950+
(struct.get $sub 0
951+
(struct.new $sub
952+
(string.const "foo")
953+
)
954+
)
955+
)
956+
)
957+
)
958+
959+
;; As above, and now the super has the field as well, preventing optimization.
960+
(module
961+
;; CHECK: (type $super (sub (struct (field (mut stringref)))))
962+
(type $super (sub (struct (field (mut stringref)))))
963+
964+
;; CHECK: (type $sub (sub $super (struct (field (mut stringref)))))
965+
(type $sub (sub $super (struct (field (mut stringref)))))
966+
967+
;; CHECK: (type $2 (func))
968+
969+
;; CHECK: (global $global (ref $super) (struct.new_default $super))
970+
(global $global (ref $super) (struct.new_default $super))
971+
;; CHECK: (export "global" (global $global))
972+
(export "global" (global $global))
973+
974+
;; CHECK: (func $test (type $2)
975+
;; CHECK-NEXT: (drop
976+
;; CHECK-NEXT: (struct.get $sub 0
977+
;; CHECK-NEXT: (struct.new $sub
978+
;; CHECK-NEXT: (string.const "foo")
979+
;; CHECK-NEXT: )
980+
;; CHECK-NEXT: )
981+
;; CHECK-NEXT: )
982+
;; CHECK-NEXT: )
983+
(func $test
984+
;; Write and read the field.
985+
(drop
986+
(struct.get $sub 0
987+
(struct.new $sub
988+
(string.const "foo")
989+
)
990+
)
991+
)
992+
)
993+
)
994+
995+
;; Two mutable fields with a chain of three subtypes. The super is public,
996+
;; preventing optimization of the field it has (but not the other; the other
997+
;; is removable anyhow, though, so this just checks for the lack of an error
998+
;; when deciding not to make the fields immutable or not).
999+
(module
1000+
;; CHECK: (type $super (sub (struct (field (mut i32)))))
1001+
(type $super (sub (struct (field (mut i32)))))
1002+
;; CHECK: (rec
1003+
;; CHECK-NEXT: (type $mid (sub $super (struct (field (mut i32)))))
1004+
(type $mid (sub $super (struct (field (mut i32)) (field (mut f64)))))
1005+
;; CHECK: (type $sub (sub $mid (struct (field (mut i32)))))
1006+
(type $sub (sub $mid (struct (field (mut i32)) (field (mut f64)))))
1007+
1008+
;; CHECK: (global $global (ref $super) (struct.new_default $sub))
1009+
(global $global (ref $super) (struct.new_default $sub))
1010+
1011+
;; CHECK: (export "global" (global $global))
1012+
(export "global" (global $global))
1013+
)
1014+

0 commit comments

Comments
 (0)