simplify types fix (fixes #107)#110
Merged
Merged
Conversation
Previously, the `simplifyType` property had the type `Type.t -> Type.t`, where `Type.unit` was used to signal an uninhabited type. However, this is ambiguous, as `Type.unit` is an inhabited type. The refactored `simplifyTypeOpt` property has the type `Type.t -> Type.t option`, where `NONE` is used to signal an uninhabited type.
Following the previous commit, further use a `Type.t option` and `NONE` to signal an uninhabited type, rather than a `Type.t` and `Type.unit`. In particular, a `Statement.t` that purports to produce a value of an uninhabited type must be unreachable. This optimization isn't expressible when an uninhabited type is signaled by `Type.unit`, because many statements may produce a value of `Type.unit`.
After identifying an unreachable statement (e.g., the `ConApp` of a `Useless` constructor), `SimplifyTypes` stops visiting subsequent statements in the block. However, visiting statements is necessary to `setVarInfo` on the variables defined in the block. Because `SimplifyTypes` visits the blocks of a function via a DFS, it may visit (necessarily unreachable) blocks that use variables that were defined (but not visited) by a previously unreachable block. This triggers a `no varInfo property` compiler error. This was previously noted (see the comment at the end of MLton/mlton@19b07c0) and has now been observed in the wild (see MPLLang#107). To fix the bug, switch from a DFS to a dominator tree traversal, where the dominator tree traversal does not visit any children of an unreachable block. This addresses the `no varInfo property` compiler error, because any block that uses a variable must be dominated by the block that defines the variable.
It is impossible for a block to be called with a value of an uninhabited type. In practice, such a block is usually the target of a `Case` transfer for a `Useless` constructor. The corresponding branch of the `Case` transfer will be eliminated.
Rather than using `unit`
It is impossible for a function to be called with a value of an uninhabited type.
It is impossible for a primitive to be called with a value of an uninhabited type. Hence, a reachable primitive must have all inhabited (and therefore useful) arguments. As a further consequence, a reachable `MLton_eq` and `MLton_equal` must have all inhabited (and therefore useful) arguments and there will be no opportunity to simplify the equality to `true`.
An array or vector of an uninhabited type is necessarily empty. This is important, because while an `Array_sub` or `Vector_sub` of a sequence of an uninhabited type will be recognized as unreachable, the "origin" of the unreachability is the dominating bounds check that must necessarily fail. Replacing `Array_length` and `Vector_length` of a sequence of an uninhabited type allows the dominating bounds check to be simplified.
It is impossible to `ConApp`, `Call`, `Raise`, or `Return` with a value of an uninhabited type. Hence, a reachable `ConApp`, `Call`, `Raise`, or `Return` must have all inhabited (and therefore useful) arguments.
In a previous commit, it was noted that it is impossible for a block
to be called with a value of an uninhabited type. This was used to
justify not including such a block in the transformed function. One
example is a block that is the target of a `Case` transfer for a
`Useless` constructor. The corresponding branch of the `Case`
transfer will be eliminated and the block will not have a use in the
transformed function.
However, it is possible for such a block to be the `cont` or `handle`
of a non-tail call. While it will be impossible for the called
function to `Return` or `Raise` with a value of an uninhabited type,
the type of the function must be (mostly) preserved. In particular,
the `returns` and `raises` are transformed via:
val returns = Option.map (returns, keepSimplifyTypes)
val raises = Option.map (raises, keepSimplifyTypes)
where `keepSimplifyTypes` drops uninhabited components. Because the
`returns` and `raises` of the function are not eliminated, the `cont`
and `handle` blocks cannot be eliminated. On the other hand, when the
block is the target of a `Case` transfer for a `Useless` constructor,
then there will be no use of the block in the transformed program and
the block will be eliminated by `Shrink`.
While it would be possible to adjust the `cont` and `handle` at a
`Call`, it is non-trivial. For example, a function that purports to
return a value of an uninhabited type cannot actually `Return`, but
could continue to `Raise`. However, the SSA IR cannot express a
non-tail call with a `handle` but no `cont`, so some dummy `cont`
block would nonetheless be required. Moreover, it would require
inspecting the `returns` and `raises` types of the called function,
which are not currently recorded in a property for `SimplifyTypes`.
Finally, this kind of calling-convention adjustment is suitably
handled by `RemoveUnused`.
It is impossible to encounter a `ConRep.FFI` `ConApp`, because all `ConRep.FFI` constructors were converted to `ConRep.Useful`. It is impossible to encounter a `ConRep.Useless` `ConApp`, because it such constructors were deemed useless.
All statements are either dead or kept (and none are deleted), so a simple option suffices.
`MLton_eq` and `MLton_equal` at a type of cardinality 1 must return `true`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the solution Matthew implemented to fix the
simplifyTypesbug (#107) which was also present in mainline MLton (MLton/mlton#367) but had only been observed with MPL.