Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SubtypingDiscoverer: Differentiate non-flow subtyping constraints #6344

Merged
merged 4 commits into from
Feb 27, 2024
Merged

SubtypingDiscoverer: Differentiate non-flow subtyping constraints #6344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f2abee6
bad.test
kripken Feb 24, 2024
89079ca
test.good
kripken Feb 24, 2024
783ef3a
fix
kripken Feb 24, 2024
093db1b
work
kripken Feb 24, 2024
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
20 changes: 18 additions & 2 deletions src/ir/subtype-exprs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,29 @@ namespace wasm {
// * noteCast(Expression, Expression) - An expression's type is cast to
// another, for example, in RefCast.
//
// In addition, we need to differentiate two situations that cause subtyping:
// * Flow-based subtyping: E.g. when a value flows out from a block, in which
// case the value must be a subtype of the block's type.
// * Non-flow-based subtyping: E.g. in RefEq, being in one of the arms means
// you must be a subtype of eqref, but your value does not flow anywhere,
// because it is processed by the RefEq and does not send it anywhere.
// The difference between the two matters in some users of this class, and so
// the above functions all handle flow-based subtyping, while there is also the
// following:
//
// * noteNonFlowSubtype(Expression, Type)
//
// This is the only signature we need for the non-flowing case since it always
// stems from an expression that is compared against a type.
//
// The concrete signatures are:
//
// void noteSubtype(Type, Type);
// void noteSubtype(HeapType, HeapType);
// void noteSubtype(Type, Expression*);
// void noteSubtype(Expression*, Type);
// void noteSubtype(Expression*, Expression*);
// void noteNonFlowSubtype(Expression*, Type);
// void noteCast(HeapType, HeapType);
// void noteCast(Expression*, Type);
// void noteCast(Expression*, Expression*);
Expand Down Expand Up @@ -202,8 +218,8 @@ struct SubtypingDiscoverer : public OverriddenVisitor<SubType> {
void visitRefIsNull(RefIsNull* curr) {}
void visitRefFunc(RefFunc* curr) {}
void visitRefEq(RefEq* curr) {
self()->noteSubtype(curr->left, Type(HeapType::eq, Nullable));
self()->noteSubtype(curr->right, Type(HeapType::eq, Nullable));
self()->noteNonFlowSubtype(curr->left, Type(HeapType::eq, Nullable));
self()->noteNonFlowSubtype(curr->right, Type(HeapType::eq, Nullable));
}
void visitTableGet(TableGet* curr) {}
void visitTableSet(TableSet* curr) {
Expand Down
4 changes: 4 additions & 0 deletions src/passes/StringLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,10 @@ struct StringLowering : public StringGathering {
// Only the type matters of the place we assign to.
noteSubtype(a, b->type);
}
void noteNonFlowSubtype(Expression* a, Type b) {
// Flow or non-flow is the same for us.
noteSubtype(a, b);
}
void noteCast(HeapType, HeapType) {
// Casts do not concern us.
}
Expand Down
16 changes: 16 additions & 0 deletions src/passes/Unsubtyping.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,22 @@ struct Unsubtyping
noteSubtype(sub->type, super->type);
}

void noteNonFlowSubtype(Expression* sub, Type super) {
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, nothing about Unsubtyping is flow-sensitive in any way, so differentiating between non-flow subtyping and flow subtyping seems like the wrong abstraction, or rather a fix for a problem we do not have here.

Is it the case that noteSubtype was simply never passed a basic supertype before that change to SubtypeDiscoverer? If so, I suspect the proper fix is to change isBottom() to isBasic() in the check at the beginning of noteSubtype.

Copy link
Member Author

Choose a reason for hiding this comment

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

Heh, our thinking is identical: I actually considered just that, but sadly it doesn't work: we do need to consider basic types there. Consider if we write a user type to anyref, then that interacts with casts. The cast logic basically needs to know about all the things that might flow into anyref, since it needs to preserve cast differences among them, when we cast back from anyref. That is, unfortunately, where the flow sensitivity comes from.

(And for that reason the new test in this PR must contain casts.)

Copy link
Member Author

Choose a reason for hiding this comment

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

(friendly ping @tlively , there is at least one project that regressed by this that is eager to get a fix)

Copy link
Member

Choose a reason for hiding this comment

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

Oops, sorry for letting this slip.

// This expression's type must be a subtype of |super|, but the value does
// not flow anywhere - this is a static constraint. As the value does not
// flow, it cannot reach anywhere else, which means we need this in order to
// validate but it does not interact with casts. Given that, if super is a
// basic type then we can simply ignore this: we only remove subtyping
// between user types, so subtyping wrt basic types is unchanged, and so
// this constraint will never be a problem.
if (super.isRef() && super.getHeapType().isBasic()) {
return;
}

// Otherwise, we must take this into account.
noteSubtype(sub, super);
}

void noteCast(HeapType src, HeapType dest) {
if (src == dest || dest.isBottom()) {
return;
Expand Down
103 changes: 103 additions & 0 deletions test/lit/passes/unsubtyping-casts.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,109 @@
)
)

;; As above, but now with some ref.eq added. Those should not inhibit
;; optimizations: as before, $bot no longer needs to subtype from $mid (but
;; $mid must subtype from $top). ref.eq does add a requirement on subtyping
;; (that the type be a subtype of eq), but ref.eq does not actually flow the
;; inputs it receives anywhere, so that doesn't stop us from removing subtyping
;; from user types.
(module
(rec
;; CHECK: (rec
;; CHECK-NEXT: (type $top (sub (struct )))
(type $top (sub (struct)))
;; CHECK: (type $mid (sub $top (struct )))
(type $mid (sub $top (struct)))
;; CHECK: (type $bot (sub (struct )))
(type $bot (sub $mid (struct)))
)

;; CHECK: (type $3 (func (param anyref (ref $top) (ref $mid) (ref $bot))))

;; CHECK: (func $cast (type $3) (param $any anyref) (param $top (ref $top)) (param $mid (ref $mid)) (param $bot (ref $bot))
;; CHECK-NEXT: (local $l anyref)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (ref.eq
;; CHECK-NEXT: (local.get $top)
;; CHECK-NEXT: (local.get $mid)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (ref.eq
;; CHECK-NEXT: (local.get $top)
;; CHECK-NEXT: (local.get $bot)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (ref.eq
;; CHECK-NEXT: (local.get $mid)
;; CHECK-NEXT: (local.get $bot)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (ref.cast (ref $bot)
;; CHECK-NEXT: (local.get $any)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (ref.cast (ref $top)
;; CHECK-NEXT: (local.get $any)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (ref.cast (ref $mid)
;; CHECK-NEXT: (local.get $any)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $l
;; CHECK-NEXT: (struct.new_default $mid)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $cast (param $any anyref) (param $top (ref $top)) (param $mid (ref $mid)) (param $bot (ref $bot))
(local $l anyref)
(drop
(ref.eq
(local.get $top)
(local.get $mid)
)
)
(drop
(ref.eq
(local.get $top)
(local.get $bot)
)
)
(drop
(ref.eq
(local.get $mid)
(local.get $bot)
)
)
(drop
;; Cast any -> $bot
(ref.cast (ref $bot)
(local.get $any)
)
)
(drop
;; Cast any -> $top
(ref.cast (ref $top)
(local.get $any)
)
)
(drop
;; Cast any -> $mid
(ref.cast (ref $mid)
(local.get $any)
)
)

(local.set $l
(struct.new $mid)
)
)
)

(module
(rec
;; CHECK: (rec
Expand Down
Loading