diff options
| -rw-r--r-- | src/passes/TypeRefining.cpp | 104 | ||||
| -rw-r--r-- | test/lit/passes/type-refining.wast | 130 |
2 files changed, 172 insertions, 62 deletions
diff --git a/src/passes/TypeRefining.cpp b/src/passes/TypeRefining.cpp index ee12c388d..c37a105ec 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) { @@ -270,71 +272,49 @@ struct TypeRefining : public Pass { return; } - 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; + Type newFieldType; + if (!curr->ref->type.isNull()) { + 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; } }; diff --git a/test/lit/passes/type-refining.wast b/test/lit/passes/type-refining.wast index 91f61bc15..fad016e1d 100644 --- a/test/lit/passes/type-refining.wast +++ b/test/lit/passes/type-refining.wast @@ -1574,3 +1574,133 @@ (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: (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 "never" (global $never)) + + ;; 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 (which avoids the pass + ;; early-exiting). + (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) + ) + ) + ) + ) + ) +) + +;; 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) + ) + ) + ) +) |