SubtypingDiscoverer: Differentiate non-flow subtyping constraints #6344
Conversation
|
Open to other ideas here, btw... it's late on Friday and this is the best I could come up with 😄 edit: e.g. maybe there is a more targeted solution in Unsubtyping? Though I suspect some changes in SubtypingDiscoverer are unavoidable. |
| @@ -193,6 +193,22 @@ struct Unsubtyping | |||
| noteSubtype(sub->type, super->type); | |||
| } | |||
|
|
|||
| void noteNonFlowSubtype(Expression* sub, Type super) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
(friendly ping @tlively , there is at least one project that regressed by this that is eager to get a fix)
There was a problem hiding this comment.
Oops, sorry for letting this slip.
…bAssembly#6344) When we do a local.set of a value into a local then we have both a subtyping constraint - for the value to be valid to put in that local - and also a flow of a value, which can then reach more places. Such flow then interacts with casts in Unsubtyping, since it needs to know what can flow where in order to know how casts force us to keep subtyping relations. That regressed in the not-actually-NFC WebAssembly#6323 in which I added the innocuous lines to add subtyping constraints in ref.eq. It seems fine to require that the arms of a RefEq must be of type eqref, but Unsubtyping then assuming those arms flowed into a location of type eqref... which means casts might force us to not optimize some things. To fix this, differentiate the rare case of non-flowing subtyping constraints, which is basically only RefEq. There are perhaps a few more cases (like i31 operations) but they do not matter in practice for Unsubtyping anyhow; I suggest we land this first to undo the regression and then at our leisure investigate the other instructions.
When we do a
local.setof a value into a local then we have both a subtyping constraint - forthe value to be valid to put in that local - and also a flow of a value, which can then reach
more places. Such flow then interacts with casts in Unsubtyping, since it needs to know
what can flow where in order to know how casts force us to keep subtyping relations.
That regressed in the not-actually-NFC #6323 in which I added the innocuous lines
to add subtyping constraints in
ref.eq. It seems fine to require that the arms of aRefEq must be of type eqref, but Unsubtyping then assuming those arms flowed into
a location of type eqref... which means casts might force us to not optimize some
things.
To fix this, differentiate the rare case of non-flowing subtyping constraints, which is
basically only RefEq. There are perhaps a few more cases (like i31 operations) but they
do not matter in practice for Unsubtyping anyhow; I suggest we land this first to undo
the regression and then at our leisure investigate the other instructions.