SI-5830 switches: support guards, unreachability -- review by @dragos #821
Conversation
This file contains 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
turn switches with guards into guard-free switches by collecting all cases
that are (possibly) guarded by different guards but that switch on the same constant,
and pushing the implied if-then-else into the collapsed case body
```
case C if G1 => B1
case C if Gi => Bi
case C if GN => BN
```
becomes
```
case C => if (G1) B1
else if (Gi) Bi
else if (GN) BN
else default() // not necessary if GN == EmptyTree
```
default() is a jump to the default case; to enable this, we wrap a default() { } labeldef
around the last case's body (the user-defined default or the synthetic case that throws the matcherror)
so we can jump to the default case after the last guard is checked
(assuming unreachability is checked, once we ended up in a non-default case,
one of the guards either matches or we go to the default case)
the unreachability analysis is minimal -- we simply check (after rewriting to guard-free form)
that:
- there are no duplicate cases
- the default case comes last
misc notes:
- can't jump in exception handlers (TODO: a more fine-grained analysis on when we need to jump)
- work around SI-6015 (test file run/t5830.scala crashed the inliner)
- propagate type of case body to label def of default case
(needed for existentials, see e.g., t2683)
- the default(){} LabelDef breaks SelectiveANFTransform -- workaround: don't write guarded switches in CPS code
(do the above transformation manually)
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/409/ |
|
jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/409/ |
|
looks good, thanks! |
adriaanm
added a commit
that referenced
this pull request
Jul 5, 2012
SI-5830 switches: support guards, unreachability
adriaanm
added a commit
to adriaanm/scala
that referenced
this pull request
Jul 11, 2012
A complete overhaul. The original implementation in SI-5830 (scala#821) was pretty buggy. Collapse guarded cases that switch on the same constant (the last case may be unguarded). Cases with patterns A and B switch on the same constant iff for all values x that match A also match B and vice versa. (This roughly corresponds to equality on trees modulo alpha renaming and reordering of alternatives.) The rewrite only applies if some of the cases are guarded (this must be checked before invoking this method). The rewrite proceeds by grouping the cases into subsequences of overlapping cases (since for any input, and when disregarding their guards, they either all match or they don't). They are then merged into one case with their guards pushed into the body as follows (with P representing the overlapping pattern): `{case P if(G_i) => B_i }*` is rewritten to `case P => {if(G_i) B_i}*` The rewrite fails (and returns Nil) when: 1. there is a subsequence of overlapping cases that has an unguarded case in the middle; only the last case may be un-guarded 2. there are overlapping cases that differ (tested by `caseImplies`) cases with patterns A and B are overlapping if for SOME value x, A matches x implies B matches y OR vice versa <-- note the difference with case equality defined above for example `case 'a' | 'b' =>` and `case 'b' =>` are different and overlapping (overlapping and equality disregard guards) Also closes SI-6048 (duplicate).
This was referenced Jul 11, 2012
adriaanm
added a commit
to adriaanm/scala
that referenced
this pull request
Jul 16, 2012
A complete overhaul. The original implementation in SI-5830 (scala#821) was pretty buggy. [from the doc comment of `collapseGuardedCases`:] Collapse guarded cases that switch on the same constant (the last case may be unguarded). Cases with patterns A and B switch on the same constant iff for all values x that match A also match B and vice versa. (This roughly corresponds to equality on trees modulo alpha renaming and reordering of alternatives.) The rewrite only applies if some of the cases are guarded (this must be checked before invoking this method). The rewrite goes through the switch top-down and merges each case with the subsequent cases it is implied by (i.e. it matches if they match, not taking guards into account) If there are no unreachable cases, all cases can be uniquely assigned to a partition of such 'overlapping' cases, save for the default case (thus we jump to it rather than copying it several times). (The cases in a partition are implied by the principal element of the partition.) The overlapping cases are merged into one case with their guards pushed into the body as follows (with P the principal element of the overlapping patterns Pi): `{case Pi if(G_i) => B_i }*` is rewritten to `case P => {if(G_i) B_i}*` The rewrite fails (and returns Nil) when: (1) there is a subsequence of overlapping cases that has an unguarded case in the middle; only the last case of each subsequence of overlapping cases may be unguarded (this is implied by unreachability) (2) there are overlapping cases that differ (tested by `caseImpliedBy`) cases with patterns A and B are overlapping if for SOME value x, A matches x implies B matches y OR vice versa <-- note the difference with case equality defined above for example `case 'a' | 'b' =>` and `case 'b' =>` are different and overlapping (overlapping and equality disregard guards) Improved by @retronym's feedback in the following ways: - fix patternEquals (it's now quadratic, but correct) - update neg/t6011 to test the improved patternEquals - remove side-effect-in-condition ugliness - introduce isGuardedCase - docs & various code clarity Also closes SI-6048 (duplicate).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
turn switches with guards into guard-free switches by collecting all cases
that are (possibly) guarded by different guards but that switch on the same constant,
and pushing the implied if-then-else into the collapsed case body
becomes
default() is a jump to the default case; to enable this, we wrap a default() { } labeldef
around the last case's body (the user-defined default or the synthetic case that throws the matcherror)
so we can jump to the default case after the last guard is checked
(assuming unreachability is checked, once we ended up in a non-default case,
one of the guards either matches or we go to the default case)
the unreachability analysis is minimal -- we simply check (after rewriting to guard-free form)
that:
misc notes:
(needed for existentials, see e.g., t2683)
(do the above transformation manually)