Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 5 additions & 5 deletions src/ir/type-updating.h
Original file line number Diff line number Diff line change
Expand Up @@ -580,11 +580,11 @@ Expression* fixLocalGet(LocalGet* get, Module& wasm);
// Applies new types of parameters to a function. This does all the necessary
// changes aside from altering the function type, which the caller is expected
// to do after we run (the caller might simply change the type, but in other
// cases the caller might be rewriting the types and need to preserve their
// identity in terms of nominal typing, so we don't change the type here). The
// specific things this function does are to update the types of local.get/tee
// operations, refinalize, etc., basically all operations necessary to ensure
// validation with the new types.
// cases the caller might be rewriting the types and need to preserve their
// identities, so we don't change the type here). The specific things this
// function does are to update the types of local.get/tee operations,
// refinalize, etc., basically all operations necessary to ensure validation
// with the new types.
//
// While doing so, we can either update or not update the types of local.get and
// local.tee operations. (We do not update them here if we'll be doing an update
Expand Down
62 changes: 22 additions & 40 deletions src/passes/OptimizeInstructions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5726,65 +5726,47 @@ struct OptimizeInstructions
// curr, and so they must be compatible to allow for a proper new
// type after the transformation.
//
// At minimum an LUB is required, as shown here:
// The children must have a shared supertype. For example, we cannot
// fold out the `drop` here because there would be no valid result
// type for the if afterward:
//
// (if
// (condition)
// (drop (i32.const 1))
// (drop (f64.const 2.0))
// )
//
// However, that may not be enough, as with nominal types we can
// have things like this:
// However, having a shared supertype may not be enough. If $A and
// $B have a shared supertype, but that supertype does not have a
// field at index 1, then we cannot fold the duplicated struct.get
// here:
//
// (if
// (condition)
// (struct.get $A 1 (..))
// (struct.get $B 1 (..))
// )
//
// It is possible that the LUB of $A and $B does not contain field
// "1". With structural types this specific problem is not possible,
// and it appears to be the case that with the GC MVP there is no
// instruction that poses a problem, but in principle it can happen
// there as well, if we add an instruction that returns the number
// of fields in a type, for example. For that reason, and to avoid
// a difference between structural and nominal typing here, disallow
// subtyping in both. (Note: In that example, the problem only
// happens because the type is not part of the struct.get - we infer
// it from the reference. That is why after hoisting the struct.get
// out, and computing a new type for the if that is now the child of
// the single struct.get, we get a struct.get of a supertype. So in
// principle we could fix this by modifying the IR as well, but the
// problem is more general, so avoid that.)
// (Note: In that example, the problem only happens because the type
// is not part of the struct.get - we infer it from the reference.
// That is why after hoisting the struct.get out, and computing a
// new type for the if that is now the child of the single
// struct.get, we get a struct.get of a supertype. So in principle
// we could fix this by modifying the IR as well, but the problem is
// more general, so avoid that.)
//
// For now, only support the case where the types of the children
// are the same.
//
// TODO: We could analyze whether the LUB of the children types
// would satisfy the constraints imposed by the shared parent
// instruction. This would require a version of ChildTyper that
// allows for generalizing type annotations.
ChildIterator ifFalseChildren(curr->ifFalse);
auto* ifTrueChild = *ifTrueChildren.begin();
auto* ifFalseChild = *ifFalseChildren.begin();
bool validTypes = ifTrueChild->type == ifFalseChild->type;

// In addition, after we move code outside of curr then we need to
// not change unreachability - if we did, we'd need to propagate
// that further, and we leave such work to DCE and Vacuum anyhow.
// This can happen in something like this for example, where the
// outer type changes from i32 to unreachable if we move the
// returns outside:
//
// (if (result i32)
// (local.get $x)
// (return
// (local.get $y)
// )
// (return
// (local.get $z)
// )
// )
assert(curr->ifTrue->type == curr->ifFalse->type);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not clear on why this assertion isn't needed? And why the if below it as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

This assertion isn't wrong, but it's not contributing much value after we remove the code below. The code below isn't necessary:

  • if newOuterType is unreachable, then both arms of the if or select must be unreachable, which means that curr->type is also unreachable and the branch will not be taken.
  • Otherwise newOuterType is not unreachable and neither arm is unreachable. In this case curr->type can only be unreachable if the if or select condition is unreachable. But since we're not changing the condition, the if or select will remain unreachable after optimizing and that unreachability will bubble up so that the curr->type continues to be unreachable after the optimization is applied, so there's no problem we need to avoid.

auto newOuterType = curr->ifTrue->type;
if ((newOuterType == Type::unreachable) !=
(curr->type == Type::unreachable)) {
validTypes = false;
}

// If the expression we are about to move outside has side effects,
// then we cannot do so in general with a select: we'd be reducing
// the amount of the effects as well as moving them. For an if,
Expand Down
7 changes: 3 additions & 4 deletions src/passes/SignaturePruning.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -279,13 +279,12 @@ struct SignaturePruning : public Pass {

// Create a new signature. When the TypeRewriter operates below it will
// modify the existing heap type in place to change its signature to this
// one (which preserves identity, that is, even if after pruning the new
// signature is structurally identical to another one, it will remain
// nominally different from those).
// one. TypeRewriter will also ensure that distinct types remain
// disctinct, even if they have the same signature after optimization.
newSignatures[type] = Signature(Type(newParams), sig.results);

// removeParameters() updates the type as it goes, but in this pass we
// need the type to match the other locations, nominally. That is, we need
// need the type to be updated in all locations at once. That is, we need
// all the functions of a particular type to still have the same type
// after this operation, and that must be the exact same type at the
// relevant call_refs and so forth. The TypeRewriter below will do the
Expand Down
2 changes: 1 addition & 1 deletion src/passes/pass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ void PassRegistry::registerPasses() {
"merge types to their supertypes where possible",
createTypeMergingPass);
registerPass("type-ssa",
"create new nominal types to help other optimizations",
"create new types to help other optimizations",
createTypeSSAPass);
registerPass("type-unfinalizing",
"mark all types as non-final (open)",
Expand Down
25 changes: 7 additions & 18 deletions src/wasm-type.h
Original file line number Diff line number Diff line change
Expand Up @@ -139,21 +139,13 @@ class HeapType {
// Choose an arbitrary heap type as the default.
constexpr HeapType() : HeapType(func) {}

// Construct a HeapType referring to the single canonical HeapType for the
// given signature. In nominal mode, this is the first HeapType created with
// this signature.
// Construct the single canonical HeapType for the given signature, struct,
// array, or continuation.
HeapType(Signature signature);

HeapType(Continuation cont);

// Create a HeapType with the given structure. In equirecursive mode, this may
// be the same as a previous HeapType created with the same contents. In
// nominal mode, this will be a fresh type distinct from all previously
// created HeapTypes.
// TODO: make these explicit to differentiate them.
HeapType(const Struct& struct_);
HeapType(Struct&& struct_);
HeapType(Array array);
HeapType(Continuation cont);

HeapTypeKind getKind() const;

Expand Down Expand Up @@ -193,7 +185,7 @@ class HeapType {
Array getArray() const;

// If there is a nontrivial (i.e. non-basic, one that was declared by the
// module) nominal supertype, return it, else an empty optional.
// module) supertype, return it, else an empty optional.
std::optional<HeapType> getDeclaredSuperType() const;

// As |getDeclaredSuperType|, but also handles basic types, that is, if the
Expand All @@ -206,8 +198,8 @@ class HeapType {
std::optional<HeapType> getDescribedType() const;
DescriptorChain getDescriptorChain() const;

// Return the depth of this heap type in the nominal type hierarchy, i.e. the
// number of supertypes in its supertype chain.
// Return the depth of this heap type in the type hierarchy, i.e. the number
// of supertypes in its supertype chain.
size_t getDepth() const;

// Get the bottom heap type for this heap type's hierarchy.
Expand Down Expand Up @@ -898,10 +890,7 @@ struct TypeBuilder {
};

// Returns all of the newly constructed heap types. May only be called once
// all of the heap types have been initialized with `setHeapType`. In nominal
// mode, all of the constructed HeapTypes will be fresh and distinct. In
// nominal mode, will also produce a fatal error if the declared subtype
// relationships are not valid.
// all of the heap types have been initialized with `setHeapType`.
BuildResult build();

// Utility for ergonomically using operator[] instead of explicit setHeapType
Expand Down
4 changes: 2 additions & 2 deletions test/lit/help/wasm-metadce.test
Original file line number Diff line number Diff line change
Expand Up @@ -557,8 +557,8 @@
;; CHECK-NEXT: type fields where possible
;; CHECK-NEXT: (using GUFA)
;; CHECK-NEXT:
;; CHECK-NEXT: --type-ssa create new nominal types to help
;; CHECK-NEXT: other optimizations
;; CHECK-NEXT: --type-ssa create new types to help other
;; CHECK-NEXT: optimizations
;; CHECK-NEXT:
;; CHECK-NEXT: --type-unfinalizing mark all types as non-final
;; CHECK-NEXT: (open)
Expand Down
4 changes: 2 additions & 2 deletions test/lit/help/wasm-opt.test
Original file line number Diff line number Diff line change
Expand Up @@ -589,8 +589,8 @@
;; CHECK-NEXT: type fields where possible
;; CHECK-NEXT: (using GUFA)
;; CHECK-NEXT:
;; CHECK-NEXT: --type-ssa create new nominal types to help
;; CHECK-NEXT: other optimizations
;; CHECK-NEXT: --type-ssa create new types to help other
;; CHECK-NEXT: optimizations
;; CHECK-NEXT:
;; CHECK-NEXT: --type-unfinalizing mark all types as non-final
;; CHECK-NEXT: (open)
Expand Down
4 changes: 2 additions & 2 deletions test/lit/help/wasm2js.test
Original file line number Diff line number Diff line change
Expand Up @@ -521,8 +521,8 @@
;; CHECK-NEXT: type fields where possible
;; CHECK-NEXT: (using GUFA)
;; CHECK-NEXT:
;; CHECK-NEXT: --type-ssa create new nominal types to help
;; CHECK-NEXT: other optimizations
;; CHECK-NEXT: --type-ssa create new types to help other
;; CHECK-NEXT: optimizations
;; CHECK-NEXT:
;; CHECK-NEXT: --type-unfinalizing mark all types as non-final
;; CHECK-NEXT: (open)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
;; Test that incorrect nominal types result in the expected parse errors
;; Test that missing supertypes result in the expected parse errors

;; RUN: foreach %s %t not wasm-opt -all 2>&1 | filecheck %s

Expand Down
5 changes: 2 additions & 3 deletions test/lit/passes/gto_and_cfp_in_O.wast
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@
;; RUN: foreach %s %t wasm-opt -O -all --closed-world -S -o - | filecheck %s
;; RUN: foreach %s %t wasm-opt -O -all -S -o - | filecheck %s --check-prefix OPEN_WORLD

;; Test that -O, with nominal typing + GC enabled, will run global type
;; optimization in conjunction with constant field propagation etc. But, in an
;; open world we do not run them.
;; Test that -O with GC enabled will run global type optimization in conjunction
;; with constant field propagation etc. in closed world mode only.

(module
;; OPEN_WORLD: (type $struct (sub (struct (field (mut funcref)) (field (mut i32)))))
Expand Down
Loading
Loading