From 115d9f7e0df0c8c78c1c9af8dd581a3fec30dc2e Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 5 Dec 2024 14:13:18 -0800 Subject: [PATCH 1/7] fix --- src/passes/TypeRefining.cpp | 95 +++++++++++++++---------------------- 1 file changed, 37 insertions(+), 58 deletions(-) diff --git a/src/passes/TypeRefining.cpp b/src/passes/TypeRefining.cpp index ee12c388df9..903524271a0 100644 --- a/src/passes/TypeRefining.cpp +++ b/src/passes/TypeRefining.cpp @@ -270,71 +270,50 @@ struct TypeRefining : public Pass { return; } + Type newFieldType; if (curr->ref->type.isNull()) { - // This get will trap. In theory we could leave this for later - // optimizations to do, but we must actually handle it here, because - // of the situation where this get's type is refined, and the input - // type is the result of a refining: - // - // (struct.get $A ;; should be refined to something - // (struct.get $B ;; just refined to nullref - // - // If the input has become a nullref then we can't just return out of - // this function, as we'd be leaving a struct.get of $A with the - // wrong type. But we can't find the right type since in Binaryen IR - // we use the ref's type to see what is being read, and that just - // turned into nullref. To avoid that corner case, just turn this code - // into unreachable code now, and the later refinalize will turn all - // the parents unreachable, avoiding any type-checking problems. - Builder builder(*getModule()); - replaceCurrent(builder.makeSequence(builder.makeDrop(curr->ref), - builder.makeUnreachable())); - return; + auto oldType = curr->ref->type.getHeapType(); + newFieldType = parent.finalInfos[oldType][curr->index].getLUB(); } - auto oldType = curr->ref->type.getHeapType(); - auto newFieldType = parent.finalInfos[oldType][curr->index].getLUB(); - if (Type::isSubType(newFieldType, curr->type)) { - // This is the normal situation, where the new type is a refinement of - // the old type. Apply that type so that the type of the struct.get - // matches what is in the refined field. ReFinalize will later - // propagate this to parents. - // - // Note that ReFinalize will also apply the type of the field itself - // to a struct.get, so our doing it here in this pass is usually - // redundant. But ReFinalize also updates other types while doing so, - // which can cause a problem: - // - // (struct.get $A - // (block (result (ref null $A)) - // (ref.null any) - // ) - // ) - // - // Here ReFinalize will turn the block's result into a bottom type, - // which means it won't know a type for the struct.get at that point. - // Doing it in this pass avoids that issue, as we have all the - // necessary information. (ReFinalize will still get into the - // situation where it doesn't know how to update the type of the - // struct.get, but it will just leave the existing type - it assumes - // no update is needed - which will be correct, since we've updated it - // ourselves here, before.) - curr->type = newFieldType; - } else { - // This instruction is invalid, so it must be the result of the - // situation described above: we ignored the read during our - // inference, and optimized accordingly, and so now we must remove it - // to keep the module validating. It doesn't matter what we emit here, - // since there are no struct.new or struct.sets for this type, so this - // code is logically unreachable. - // - // Note that we emit an unreachable here, which changes the type, and - // so we should refinalize. However, we will be refinalizing later - // anyhow in updateTypes, so there is no need. + if (curr->ref->type.isNull() || + newFieldType == Type::unreachable || + !Type::isSubType(newFieldType, curr->type)) { + // This get will trap, or cannot be reached: either the ref is null, + // or the field is never written any contents, or the contents we see + // are invalid (they passed through some fallthrough that will trap at + // runtime). Emit unreachable code here. Builder builder(*getModule()); replaceCurrent(builder.makeSequence(builder.makeDrop(curr->ref), builder.makeUnreachable())); + return; } + + // This is the normal situation, where the new type is a refinement of + // the old type. Apply that type so that the type of the struct.get + // matches what is in the refined field. ReFinalize will later + // propagate this to parents. + // + // Note that ReFinalize will also apply the type of the field itself + // to a struct.get, so our doing it here in this pass is usually + // redundant. But ReFinalize also updates other types while doing so, + // which can cause a problem: + // + // (struct.get $A + // (block (result (ref null $A)) + // (ref.null any) + // ) + // ) + // + // Here ReFinalize will turn the block's result into a bottom type, + // which means it won't know a type for the struct.get at that point. + // Doing it in this pass avoids that issue, as we have all the + // necessary information. (ReFinalize will still get into the + // situation where it doesn't know how to update the type of the + // struct.get, but it will just leave the existing type - it assumes + // no update is needed - which will be correct, since we've updated it + // ourselves here, before.) + curr->type = newFieldType; } }; From 11bfd28e742a74647525391bb7c4d26cb3d01576 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 5 Dec 2024 14:26:36 -0800 Subject: [PATCH 2/7] tests --- test/lit/passes/gto_and_cfp_in_O.wast | 2 +- test/lit/passes/type-refining.wast | 167 +++++++++++++++++++++----- 2 files changed, 135 insertions(+), 34 deletions(-) diff --git a/test/lit/passes/gto_and_cfp_in_O.wast b/test/lit/passes/gto_and_cfp_in_O.wast index b1e9a47df50..73e7411db5b 100644 --- a/test/lit/passes/gto_and_cfp_in_O.wast +++ b/test/lit/passes/gto_and_cfp_in_O.wast @@ -50,7 +50,7 @@ ;; CHECK: (export "main" (func $main)) ;; CHECK: (func $main (type $0) (result i32) - ;; CHECK-NEXT: (i32.const 100) + ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; OPEN_WORLD: (func $main (type $2) (result i32) ;; OPEN_WORLD-NEXT: (struct.get $struct 1 diff --git a/test/lit/passes/type-refining.wast b/test/lit/passes/type-refining.wast index 91f61bc151e..66bc83a85c6 100644 --- a/test/lit/passes/type-refining.wast +++ b/test/lit/passes/type-refining.wast @@ -26,8 +26,11 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (struct.get $struct 2 - ;; CHECK-NEXT: (local.get $struct) + ;; CHECK-NEXT: (block + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (local.get $struct) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) @@ -432,8 +435,11 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: (struct.set $struct 0 ;; CHECK-NEXT: (local.get $struct) - ;; CHECK-NEXT: (struct.get $struct 0 - ;; CHECK-NEXT: (local.get $struct) + ;; CHECK-NEXT: (block + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (local.get $struct) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) @@ -969,12 +975,18 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (struct.new $A - ;; CHECK-NEXT: (local.tee $a - ;; CHECK-NEXT: (struct.get $A 0 - ;; CHECK-NEXT: (local.get $A) + ;; CHECK-NEXT: (block ;; (replaces unreachable StructNew we can't emit) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (local.tee $a + ;; CHECK-NEXT: (block + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (local.get $A) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) @@ -1017,12 +1029,13 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: (struct.set $A 0 ;; CHECK-NEXT: (local.get $A) - ;; CHECK-NEXT: (if (result (ref $A)) + ;; CHECK-NEXT: (if ;; CHECK-NEXT: (i32.const 1) ;; CHECK-NEXT: (then - ;; CHECK-NEXT: (struct.get $A 0 + ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (local.get $A) ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (else ;; CHECK-NEXT: (unreachable) @@ -1030,18 +1043,22 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (struct.new $A - ;; CHECK-NEXT: (if (result (ref $A)) - ;; CHECK-NEXT: (i32.const 1) - ;; CHECK-NEXT: (then - ;; CHECK-NEXT: (struct.get $A 0 - ;; CHECK-NEXT: (local.get $A) + ;; CHECK-NEXT: (block ;; (replaces unreachable StructNew we can't emit) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: (then + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (local.get $A) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (else + ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (else - ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) @@ -1099,7 +1116,7 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: (struct.set $struct 0 ;; CHECK-NEXT: (local.get $struct) - ;; CHECK-NEXT: (block ;; (replaces unreachable StructGet we can't emit) + ;; CHECK-NEXT: (block ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (block (result nullref) ;; CHECK-NEXT: (ref.null none) @@ -1147,14 +1164,23 @@ ;; CHECK: (type $2 (func (result (ref $A)))) ;; CHECK: (func $0 (type $2) (result (ref $A)) - ;; CHECK-NEXT: (struct.new $A - ;; CHECK-NEXT: (ref.cast (ref $B) - ;; CHECK-NEXT: (struct.get $A 0 - ;; CHECK-NEXT: (struct.new $A - ;; CHECK-NEXT: (struct.new_default $B) + ;; CHECK-NEXT: (block ;; (replaces unreachable StructNew we can't emit) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (block ;; (replaces unreachable RefCast we can't emit) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (block + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (struct.new $A + ;; CHECK-NEXT: (struct.new_default $B) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) (func $0 (result (ref $A)) @@ -1193,10 +1219,13 @@ ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (block ;; (replaces unreachable StructNew we can't emit) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block + ;; CHECK-NEXT: (block ;; (replaces unreachable StructGet we can't emit) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (struct.get $B 0 - ;; CHECK-NEXT: (struct.new_default $B) + ;; CHECK-NEXT: (block + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (struct.new_default $B) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (unreachable) @@ -1257,13 +1286,14 @@ ;; CHECK-NEXT: (ref.cast (ref noextern) ;; CHECK-NEXT: (try (result externref) ;; CHECK-NEXT: (do - ;; CHECK-NEXT: (struct.get $A 0 + ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (struct.new $A ;; CHECK-NEXT: (ref.as_non_null ;; CHECK-NEXT: (ref.null noextern) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (catch $tag ;; CHECK-NEXT: (local.get $extern) @@ -1309,13 +1339,14 @@ ;; CHECK-NEXT: (ref.cast (ref noextern) ;; CHECK-NEXT: (try (result externref) ;; CHECK-NEXT: (do - ;; CHECK-NEXT: (struct.get $A 0 + ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (struct.new $A ;; CHECK-NEXT: (ref.as_non_null ;; CHECK-NEXT: (ref.null noextern) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (catch $tag ;; CHECK-NEXT: (local.get $extern) @@ -1554,10 +1585,13 @@ ;; CHECK: (func $1 (type $3) (result (ref null $8)) ;; CHECK-NEXT: (local $l (ref $9)) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (struct.get $5 0 - ;; CHECK-NEXT: (struct.new $5 - ;; CHECK-NEXT: (ref.func $1) + ;; CHECK-NEXT: (block + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (struct.new $5 + ;; CHECK-NEXT: (ref.func $1) + ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (ref.null nofunc) @@ -1574,3 +1608,70 @@ (ref.null $8) ) ) + +;; Test for a bug where we made a struct.get unreachable because it was reading +;; a field that had no writes, but in a situation where it is invalid for the +;; struct.get to be unreachable. +(module + ;; CHECK: (type $never (sub (struct (field i32)))) + (type $never (sub (struct (field i32)))) + + ;; CHECK: (rec + ;; CHECK-NEXT: (type $optimizable (struct (field (mut nullfuncref)))) + (type $optimizable (struct (field (mut (ref null func))))) + + ;; CHECK: (type $2 (func)) + + ;; CHECK: (type $3 (func (result (ref $never)))) + + ;; CHECK: (export "export" (func $export)) + + ;; CHECK: (func $setup (type $2) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (struct.new $optimizable + ;; CHECK-NEXT: (ref.null nofunc) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (block + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (block (result (ref none)) + ;; CHECK-NEXT: (ref.as_non_null + ;; CHECK-NEXT: (ref.null none) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $setup + ;; A struct.new, so that we have a field to refine. + (drop + (struct.new $optimizable + (ref.null func) + ) + ) + ;; A struct.get that reads a $never, but the actual fallthrough value is none. + ;; We never create this type, so the field has no possible content, and we can + ;; replace this with an unreachable. + (drop + (struct.get $never 0 + (block (result (ref $never)) + (ref.as_non_null + (ref.null none) + ) + ) + ) + ) + ) + + ;; CHECK: (func $export (type $3) (result (ref $never)) + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + (func $export (export "export") (result (ref $never)) + ;; Make the type $never public (if it were private, we'd optimize in a + ;; different way that avoids the bug that this tests for). + (unreachable) + ) +) From 79752b90d499b87a533e64746ad1ab96de59ba18 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 5 Dec 2024 14:29:09 -0800 Subject: [PATCH 3/7] oops --- src/passes/TypeRefining.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/passes/TypeRefining.cpp b/src/passes/TypeRefining.cpp index 903524271a0..ac9aa5b33e2 100644 --- a/src/passes/TypeRefining.cpp +++ b/src/passes/TypeRefining.cpp @@ -271,7 +271,7 @@ struct TypeRefining : public Pass { } Type newFieldType; - if (curr->ref->type.isNull()) { + if (!curr->ref->type.isNull()) { auto oldType = curr->ref->type.getHeapType(); newFieldType = parent.finalInfos[oldType][curr->index].getLUB(); } From 8f9f8e8520f813b3e573e99c7d184c7c41992bba Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 5 Dec 2024 14:31:06 -0800 Subject: [PATCH 4/7] fix --- test/lit/passes/gto_and_cfp_in_O.wast | 2 +- test/lit/passes/type-refining.wast | 100 +++++++++----------------- 2 files changed, 34 insertions(+), 68 deletions(-) diff --git a/test/lit/passes/gto_and_cfp_in_O.wast b/test/lit/passes/gto_and_cfp_in_O.wast index 73e7411db5b..b1e9a47df50 100644 --- a/test/lit/passes/gto_and_cfp_in_O.wast +++ b/test/lit/passes/gto_and_cfp_in_O.wast @@ -50,7 +50,7 @@ ;; CHECK: (export "main" (func $main)) ;; CHECK: (func $main (type $0) (result i32) - ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: (i32.const 100) ;; CHECK-NEXT: ) ;; OPEN_WORLD: (func $main (type $2) (result i32) ;; OPEN_WORLD-NEXT: (struct.get $struct 1 diff --git a/test/lit/passes/type-refining.wast b/test/lit/passes/type-refining.wast index 66bc83a85c6..b274021f809 100644 --- a/test/lit/passes/type-refining.wast +++ b/test/lit/passes/type-refining.wast @@ -26,11 +26,8 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (local.get $struct) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: (struct.get $struct 2 + ;; CHECK-NEXT: (local.get $struct) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) @@ -435,11 +432,8 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: (struct.set $struct 0 ;; CHECK-NEXT: (local.get $struct) - ;; CHECK-NEXT: (block - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (local.get $struct) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: (struct.get $struct 0 + ;; CHECK-NEXT: (local.get $struct) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) @@ -975,18 +969,12 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block ;; (replaces unreachable StructNew we can't emit) - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (local.tee $a - ;; CHECK-NEXT: (block - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (local.get $A) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (unreachable) - ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (struct.new $A + ;; CHECK-NEXT: (local.tee $a + ;; CHECK-NEXT: (struct.get $A 0 + ;; CHECK-NEXT: (local.get $A) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) @@ -1029,13 +1017,12 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: (struct.set $A 0 ;; CHECK-NEXT: (local.get $A) - ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (if (result (ref $A)) ;; CHECK-NEXT: (i32.const 1) ;; CHECK-NEXT: (then - ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (struct.get $A 0 ;; CHECK-NEXT: (local.get $A) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (else ;; CHECK-NEXT: (unreachable) @@ -1043,22 +1030,18 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block ;; (replaces unreachable StructNew we can't emit) - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (if - ;; CHECK-NEXT: (i32.const 1) - ;; CHECK-NEXT: (then - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (local.get $A) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (unreachable) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (else - ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: (struct.new $A + ;; CHECK-NEXT: (if (result (ref $A)) + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: (then + ;; CHECK-NEXT: (struct.get $A 0 + ;; CHECK-NEXT: (local.get $A) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (else + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) @@ -1116,7 +1099,7 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: (struct.set $struct 0 ;; CHECK-NEXT: (local.get $struct) - ;; CHECK-NEXT: (block + ;; CHECK-NEXT: (block ;; (replaces unreachable StructGet we can't emit) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (block (result nullref) ;; CHECK-NEXT: (ref.null none) @@ -1164,23 +1147,14 @@ ;; CHECK: (type $2 (func (result (ref $A)))) ;; CHECK: (func $0 (type $2) (result (ref $A)) - ;; CHECK-NEXT: (block ;; (replaces unreachable StructNew we can't emit) - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block ;; (replaces unreachable RefCast we can't emit) - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (struct.new $A - ;; CHECK-NEXT: (struct.new_default $B) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (unreachable) - ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (struct.new $A + ;; CHECK-NEXT: (ref.cast (ref $B) + ;; CHECK-NEXT: (struct.get $A 0 + ;; CHECK-NEXT: (struct.new $A + ;; CHECK-NEXT: (struct.new_default $B) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) (func $0 (result (ref $A)) @@ -1219,13 +1193,10 @@ ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (block ;; (replaces unreachable StructNew we can't emit) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block ;; (replaces unreachable StructGet we can't emit) + ;; CHECK-NEXT: (block ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (struct.new_default $B) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: (struct.get $B 0 + ;; CHECK-NEXT: (struct.new_default $B) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (unreachable) @@ -1286,14 +1257,13 @@ ;; CHECK-NEXT: (ref.cast (ref noextern) ;; CHECK-NEXT: (try (result externref) ;; CHECK-NEXT: (do - ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (struct.get $A 0 ;; CHECK-NEXT: (struct.new $A ;; CHECK-NEXT: (ref.as_non_null ;; CHECK-NEXT: (ref.null noextern) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (catch $tag ;; CHECK-NEXT: (local.get $extern) @@ -1339,14 +1309,13 @@ ;; CHECK-NEXT: (ref.cast (ref noextern) ;; CHECK-NEXT: (try (result externref) ;; CHECK-NEXT: (do - ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (struct.get $A 0 ;; CHECK-NEXT: (struct.new $A ;; CHECK-NEXT: (ref.as_non_null ;; CHECK-NEXT: (ref.null noextern) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (catch $tag ;; CHECK-NEXT: (local.get $extern) @@ -1585,13 +1554,10 @@ ;; CHECK: (func $1 (type $3) (result (ref null $8)) ;; CHECK-NEXT: (local $l (ref $9)) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (struct.new $5 - ;; CHECK-NEXT: (ref.func $1) - ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (struct.get $5 0 + ;; CHECK-NEXT: (struct.new $5 + ;; CHECK-NEXT: (ref.func $1) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (ref.null nofunc) From b680a3f72cc66f783dc64729de427ef229599c5d Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Thu, 5 Dec 2024 15:46:35 -0800 Subject: [PATCH 5/7] format --- src/passes/TypeRefining.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/passes/TypeRefining.cpp b/src/passes/TypeRefining.cpp index ac9aa5b33e2..4022999343f 100644 --- a/src/passes/TypeRefining.cpp +++ b/src/passes/TypeRefining.cpp @@ -276,8 +276,7 @@ struct TypeRefining : public Pass { newFieldType = parent.finalInfos[oldType][curr->index].getLUB(); } - if (curr->ref->type.isNull() || - newFieldType == Type::unreachable || + if (curr->ref->type.isNull() || newFieldType == Type::unreachable || !Type::isSubType(newFieldType, curr->type)) { // This get will trap, or cannot be reached: either the ref is null, // or the field is never written any contents, or the contents we see From 2af2c59bf958230c20e1386c9d5fcfdbe783bc52 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 9 Dec 2024 11:53:45 -0800 Subject: [PATCH 6/7] feedbac --- test/lit/passes/type-refining.wast | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/test/lit/passes/type-refining.wast b/test/lit/passes/type-refining.wast index b274021f809..f4d173601fb 100644 --- a/test/lit/passes/type-refining.wast +++ b/test/lit/passes/type-refining.wast @@ -1588,9 +1588,14 @@ ;; CHECK: (type $2 (func)) - ;; CHECK: (type $3 (func (result (ref $never)))) + ;; CHECK: (global $never (ref null $never) (ref.null none)) + (global $never (export "never") (ref null $never) + ;; Make the type $never public (if it were private, we'd optimize in a + ;; different way that avoids the bug that this tests for). + (ref.null $never) + ) - ;; CHECK: (export "export" (func $export)) + ;; CHECK: (export "never" (global $never)) ;; CHECK: (func $setup (type $2) ;; CHECK-NEXT: (drop @@ -1612,7 +1617,8 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) (func $setup - ;; A struct.new, so that we have a field to refine. + ;; A struct.new, so that we have a field to refine (which avoids the pass + ;; early-exiting). (drop (struct.new $optimizable (ref.null func) @@ -1631,13 +1637,4 @@ ) ) ) - - ;; CHECK: (func $export (type $3) (result (ref $never)) - ;; CHECK-NEXT: (unreachable) - ;; CHECK-NEXT: ) - (func $export (export "export") (result (ref $never)) - ;; Make the type $never public (if it were private, we'd optimize in a - ;; different way that avoids the bug that this tests for). - (unreachable) - ) ) From 63e023cf441c768883fcc3bbd0a85110319d2c61 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 9 Dec 2024 13:12:56 -0800 Subject: [PATCH 7/7] fix --- src/passes/TypeRefining.cpp | 8 ++-- test/lit/passes/type-refining.wast | 66 ++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/src/passes/TypeRefining.cpp b/src/passes/TypeRefining.cpp index 4022999343f..c37a105ece7 100644 --- a/src/passes/TypeRefining.cpp +++ b/src/passes/TypeRefining.cpp @@ -58,11 +58,13 @@ struct FieldInfoScanner void noteDefault(Type fieldType, HeapType type, Index index, FieldInfo& info) { - // Default values do not affect what the heap type of a field can be turned - // into. Note them, however, as they force us to keep the type nullable. + // Default values must be noted, so that we know there is content there. if (fieldType.isRef()) { - info.note(Type(fieldType.getHeapType().getBottom(), Nullable)); + // All we need to note here is nullability (the field must remain + // nullable), but not anything else about the type. + fieldType = Type(fieldType.getHeapType().getBottom(), Nullable); } + info.note(fieldType); } void noteCopy(HeapType type, Index index, FieldInfo& info) { diff --git a/test/lit/passes/type-refining.wast b/test/lit/passes/type-refining.wast index f4d173601fb..fad016e1d57 100644 --- a/test/lit/passes/type-refining.wast +++ b/test/lit/passes/type-refining.wast @@ -1638,3 +1638,69 @@ ) ) ) + +;; Test that we note default values. +(module + (rec + ;; CHECK: (rec + ;; CHECK-NEXT: (type $A (sub (struct (field i32)))) + (type $A (sub (struct (field i32)))) + ;; CHECK: (type $B (sub (struct (field i32)))) + (type $B (sub (struct (field i32)))) + ) + ;; CHECK: (type $2 (func (param (ref null $A) (ref null $B)))) + + ;; CHECK: (type $optimizable (sub (struct (field (ref $2))))) + (type $optimizable (sub (struct (field funcref)))) + + ;; CHECK: (elem declare func $test) + + ;; CHECK: (export "test" (func $test)) + + ;; CHECK: (func $test (type $2) (param $x (ref null $A)) (param $y (ref null $B)) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (struct.new $optimizable + ;; CHECK-NEXT: (ref.func $test) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (struct.get $A 0 + ;; CHECK-NEXT: (struct.new $A + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (struct.get $B 0 + ;; CHECK-NEXT: (struct.new_default $B) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $test (export "test") (param $x (ref null $A)) (param $y (ref null $B)) + ;; Use $A, $B as params of this export, so they are public. + + ;; Make something for the pass to do, to avoid early-exit. + (drop + (struct.new $optimizable + (ref.func $test) + ) + ) + + ;; Get from a struct.new. We have nothing to optimize here. (In particular, we + ;; cannot make this unreachable, as there is a value in the field, 0.) + (drop + (struct.get $A 0 + (struct.new $A + (i32.const 0) + ) + ) + ) + + ;; As above. Now the value in the field comes from a default value. + (drop + (struct.get $B 0 + (struct.new_default $B) + ) + ) + ) +)