Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v13]
On Fri, 4 Jun 2021 20:20:26 GMT, Jan Lahoda wrote: >> This is a preview of a patch implementing JEP 406: Pattern Matching for >> switch (Preview): >> https://bugs.openjdk.java.net/browse/JDK-8213076 >> >> The current draft of the specification is here: >> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html >> >> A summary of notable parts of the patch: >> -to support cases expressions and patterns in cases, there is a new common >> superinterface for expressions and patterns, `CaseLabelTree`, which >> expressions and patterns implement, and a list of case labels is returned >> from `CaseTree.getLabels()`. >> -to support `case default`, there is an implementation of `CaseLabelTree` >> that represents it (`DefaultCaseLabelTree`). It is used also to represent >> the conventional `default` internally and in the newly added methods. >> -in the parser, parenthesized patterns and expressions need to be >> disambiguated when parsing case labels. >> -Lower has been enhanced to handle `case null` for ordinary >> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed >> primitives, as there is no value that is not part of the input domain so >> that could be used to represent `case null`. Requires a bit shuffling with >> values. >> -TransPatterns has been enhanced to handle the pattern matching switch. It >> produces code that delegates to a new bootstrap method, that will classify >> the input value to the switch and return the case number, to which the >> switch then jumps. To support guards, the switches (and the bootstrap >> method) are restartable. The bootstrap method as such is written very simply >> so far, but could be much more optimized later. >> -nullable type patterns are `case String s, null`/`case null, String >> s`/`case null: case String s:`/`case String s: case null:`, handling of >> these required a few tricks in `Attr`, `Flow` and `TransPatterns`. >> >> The specdiff for the change is here (to be updated): >> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html > > Jan Lahoda has updated the pull request incrementally with two additional > commits since the last revision: > > - Applying review feedback. > - Tweaking javadoc. Dynamic constants are needed when de-structuring classes that are not record at top-level, to make the type that will carry the bindings, from invokedynamic to where they are accessed, opaque. So dynamic constants are not needed yet ! - PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 20:17:43 GMT, Jan Lahoda wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixing typo. > > Thanks a lot for all the feedback. I've tried to do the requested changes in > the recent commits. @lahodaj I also noticed that https://bugs.openjdk.java.net/browse/JDK-8213076 as well as https://openjdk.java.net/jeps/406 state > The implementation will likely make use of dynamic constants (JEP 309). and wondering if this should be changed on > The implementation will likely make use of invokedynamic. or maybe even removed? - PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 18:23:28 GMT, Vicente Romero wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixing typo.
>
> test/langtools/tools/javac/patterns/SealedTypeChanges.java line 71:
>
>> 69: }
>> 70:
>> 71: sealed interface SealedTypeChangesIntf permits SealedTypeChanges.A {}
>
> just for completeness shouldn't we have a test with sealed, non-abstract
> classes?
Note that for sealed non-abstract classes, the permits is not checked (as an
instance of the non-abstract class may be created and passed to the switch, the
switch needs to contain a case that will cover the class anyway). I've added
tests that check the behavior for abstract class, and non-abstract classes
(error is produced in the latter case).
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v13]
> This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html Jan Lahoda has updated the pull request incrementally with two additional commits since the last revision: - Applying review feedback. - Tweaking javadoc. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3863/files - new: https://git.openjdk.java.net/jdk/pull/3863/files/8d4c02b4..e3c29759 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3863=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3863=11-12 Stats: 125 lines in 8 files changed: 91 ins; 10 del; 24 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 15:46:32 GMT, Paul Sandoz wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixing typo.
>
> test/langtools/tools/javac/patterns/DisambiguateParenthesizedPattern.java
> line 72:
>
>> 70: SwitchTree st = (SwitchTree)
>> method.getBody().getStatements().get(0);
>> 71: CaseLabelTree label = st.getCases().get(0).getLabels().get(0);
>> 72: ExpressionType actualType = switch (label) {
>
> Should the test be careful of using a pattern match switch?
I don't think using the new feature in the tests is problematic (esp. javac
tests related to the feature). It helps to ensure the feature really works on
real code.
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 09:46:31 GMT, Jan Lahoda wrote: >> This is a preview of a patch implementing JEP 406: Pattern Matching for >> switch (Preview): >> https://bugs.openjdk.java.net/browse/JDK-8213076 >> >> The current draft of the specification is here: >> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html >> >> A summary of notable parts of the patch: >> -to support cases expressions and patterns in cases, there is a new common >> superinterface for expressions and patterns, `CaseLabelTree`, which >> expressions and patterns implement, and a list of case labels is returned >> from `CaseTree.getLabels()`. >> -to support `case default`, there is an implementation of `CaseLabelTree` >> that represents it (`DefaultCaseLabelTree`). It is used also to represent >> the conventional `default` internally and in the newly added methods. >> -in the parser, parenthesized patterns and expressions need to be >> disambiguated when parsing case labels. >> -Lower has been enhanced to handle `case null` for ordinary >> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed >> primitives, as there is no value that is not part of the input domain so >> that could be used to represent `case null`. Requires a bit shuffling with >> values. >> -TransPatterns has been enhanced to handle the pattern matching switch. It >> produces code that delegates to a new bootstrap method, that will classify >> the input value to the switch and return the case number, to which the >> switch then jumps. To support guards, the switches (and the bootstrap >> method) are restartable. The bootstrap method as such is written very simply >> so far, but could be much more optimized later. >> -nullable type patterns are `case String s, null`/`case null, String >> s`/`case null: case String s:`/`case String s: case null:`, handling of >> these required a few tricks in `Attr`, `Flow` and `TransPatterns`. >> >> The specdiff for the change is here (to be updated): >> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Fixing typo. Thanks a lot for all the feedback. I've tried to do the requested changes in the recent commits. - PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 09:46:31 GMT, Jan Lahoda wrote:
>> This is a preview of a patch implementing JEP 406: Pattern Matching for
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>>
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>>
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common
>> superinterface for expressions and patterns, `CaseLabelTree`, which
>> expressions and patterns implement, and a list of case labels is returned
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree`
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed
>> primitives, as there is no value that is not part of the input domain so
>> that could be used to represent `case null`. Requires a bit shuffling with
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It
>> produces code that delegates to a new bootstrap method, that will classify
>> the input value to the switch and return the case number, to which the
>> switch then jumps. To support guards, the switches (and the bootstrap
>> method) are restartable. The bootstrap method as such is written very simply
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String
>> s`/`case null: case String s:`/`case String s: case null:`, handling of
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>>
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixing typo.
test/langtools/tools/javac/patterns/SealedTypeChanges.java line 58:
> 56: void statement(SealedTypeChangesIntf obj) {
> 57: switch (obj) {
> 58: case A a -> System.err.println(1);
what about having tests with a case that matches the sealed class?
test/langtools/tools/javac/patterns/SealedTypeChanges.java line 71:
> 69: }
> 70:
> 71: sealed interface SealedTypeChangesIntf permits SealedTypeChanges.A {}
just for completeness shouldn't we have a test with sealed, non-abstract
classes?
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 09:46:31 GMT, Jan Lahoda wrote:
>> This is a preview of a patch implementing JEP 406: Pattern Matching for
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>>
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>>
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common
>> superinterface for expressions and patterns, `CaseLabelTree`, which
>> expressions and patterns implement, and a list of case labels is returned
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree`
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed
>> primitives, as there is no value that is not part of the input domain so
>> that could be used to represent `case null`. Requires a bit shuffling with
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It
>> produces code that delegates to a new bootstrap method, that will classify
>> the input value to the switch and return the case number, to which the
>> switch then jumps. To support guards, the switches (and the bootstrap
>> method) are restartable. The bootstrap method as such is written very simply
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String
>> s`/`case null: case String s:`/`case String s: case null:`, handling of
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>>
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixing typo.
I reviewed the `java.base` change namely, SwitchBootstraps.java. Looks good.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 47:
> 45: * of the {@code switch}, implicitly numbered sequentially from {@code
> [0..N)}.
> 46: *
> 47: * The bootstrap call site accepts a single parameter of the type of
> the
It takes 2 parameters (not single parameter). Perhaps you can take out this
paragraph since it's specified in the `typeSwitch` method.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 110:
> 108: * @return a {@code CallSite} returning the first matching element
> as described above
> 109: *
> 110: * @throws NullPointerException if any argument is null
Suggestion:
* @throws NullPointerException if any argument is {@code null}
same formatting nit for other occurrenace of "null"
-
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 09:46:31 GMT, Jan Lahoda wrote:
>> This is a preview of a patch implementing JEP 406: Pattern Matching for
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>>
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>>
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common
>> superinterface for expressions and patterns, `CaseLabelTree`, which
>> expressions and patterns implement, and a list of case labels is returned
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree`
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed
>> primitives, as there is no value that is not part of the input domain so
>> that could be used to represent `case null`. Requires a bit shuffling with
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It
>> produces code that delegates to a new bootstrap method, that will classify
>> the input value to the switch and return the case number, to which the
>> switch then jumps. To support guards, the switches (and the bootstrap
>> method) are restartable. The bootstrap method as such is written very simply
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String
>> s`/`case null: case String s:`/`case String s: case null:`, handling of
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>>
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixing typo.
A really nice feature, and a significant amount of work in javac. I mostly
focused on the bootstrap and API aspects, and left some minor comments (most of
which you can choose to apply or not as you see fit).
I suspect the bootstrap might evolve as we get feedback and switch is enhanced
with further forms of matching. But, at the moment it looks good.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 87:
> 85: * returns {@literal -1}.
> 86: *
> 87: * the {@code target} is not {@code null}, then the method of the
> call site
Suggestion:
* If the {@code target} is not {@code null}, then the method of the call
site
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 92:
> 90: *
> 91: * the element is of type {@code Class} and the target value
> 92: * is a subtype of this {@code Class}; or
Suggestion:
* the element is of type {@code Class} that is assignable
* from the target's class; or
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 162:
> 160: return i;
> 161: } else {
> 162: if (label instanceof Integer constant) {
Minor suggestion: use `else if` rather than nest
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 166:
> 164: return i;
> 165: }
> 166: if (target instanceof Character input &&
> constant.intValue() == input.charValue()) {
Use `else if`
src/jdk.compiler/share/classes/com/sun/source/tree/CaseLabelTree.java line 31:
> 29:
> 30: /**A marker interface for {@code Tree}s that may be used as {@link
> CaseTree} labels.
> 31: *
Suggestion:
/**
* A marker interface for {@code Tree}s that may be used as {@link CaseTree}
labels.
*
src/jdk.compiler/share/classes/com/sun/source/tree/DefaultCaseLabelTree.java
line 30:
> 28:
> 29: /** A case label that marks {@code default} in {@code case null, default}.
> 30: * @since 17
Suggestion:
/**
* A case label that marks {@code default} in {@code case null, default}.
*
* @since 17
test/jdk/java/lang/runtime/SwitchBootstrapsTest.java line 55:
> 53: catch (NoSuchMethodException | IllegalAccessException e) {
> 54: throw new RuntimeException(e);
> 55: }
Suggestion:
catch (ReflectiveOperationException e) {
throw new AssertionError(e, "Should not happen");
}
test/jdk/java/lang/runtime/SwitchBootstrapsTest.java line 73:
> 71: }
> 72:
> 73: public void testTypes() throws Throwable {
As a follow up issue consider adding tests for `null` values
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
> This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Fixing typo. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3863/files - new: https://git.openjdk.java.net/jdk/pull/3863/files/216b87c2..8d4c02b4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3863=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3863=10-11 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v11]
On Fri, 4 Jun 2021 09:01:27 GMT, Jan Lahoda wrote: >> This is a preview of a patch implementing JEP 406: Pattern Matching for >> switch (Preview): >> https://bugs.openjdk.java.net/browse/JDK-8213076 >> >> The current draft of the specification is here: >> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html >> >> A summary of notable parts of the patch: >> -to support cases expressions and patterns in cases, there is a new common >> superinterface for expressions and patterns, `CaseLabelTree`, which >> expressions and patterns implement, and a list of case labels is returned >> from `CaseTree.getLabels()`. >> -to support `case default`, there is an implementation of `CaseLabelTree` >> that represents it (`DefaultCaseLabelTree`). It is used also to represent >> the conventional `default` internally and in the newly added methods. >> -in the parser, parenthesized patterns and expressions need to be >> disambiguated when parsing case labels. >> -Lower has been enhanced to handle `case null` for ordinary >> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed >> primitives, as there is no value that is not part of the input domain so >> that could be used to represent `case null`. Requires a bit shuffling with >> values. >> -TransPatterns has been enhanced to handle the pattern matching switch. It >> produces code that delegates to a new bootstrap method, that will classify >> the input value to the switch and return the case number, to which the >> switch then jumps. To support guards, the switches (and the bootstrap >> method) are restartable. The bootstrap method as such is written very simply >> so far, but could be much more optimized later. >> -nullable type patterns are `case String s, null`/`case null, String >> s`/`case null: case String s:`/`case String s: case null:`, handling of >> these required a few tricks in `Attr`, `Flow` and `TransPatterns`. >> >> The specdiff for the change is here (to be updated): >> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html > > Jan Lahoda has updated the pull request incrementally with two additional > commits since the last revision: > > - Tweaking SwitchBootstraps javadoc, as suggested. > - Improving javadoc. Re-approving to keep the bots happy - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v11]
> This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html Jan Lahoda has updated the pull request incrementally with two additional commits since the last revision: - Tweaking SwitchBootstraps javadoc, as suggested. - Improving javadoc. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3863/files - new: https://git.openjdk.java.net/jdk/pull/3863/files/fa50b5fb..216b87c2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3863=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3863=09-10 Stats: 66 lines in 2 files changed: 40 ins; 9 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v10]
> This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 19 commits: - Merging master into JDK-8262891 - Enhancing tests as suggested. - Fixing enum switch with patterns with guards. - Fixing tests. - Total pattern dominates the null pattern. - Properly report errors for pattern+default clash. - Avoiding unnecessary StackMap point. - Post-merge fix - need to include jdk.internal.javac in the list of packages used by jdk.compiler again, as we now (again) have a preview feature in javac. - Correcting LineNumberTable for rule switches. - Merging master into JDK-8262891 - ... and 9 more: https://git.openjdk.java.net/jdk/compare/bdeaeb47...fa50b5fb - Changes: https://git.openjdk.java.net/jdk/pull/3863/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3863=09 Stats: 4828 lines in 79 files changed: 4509 ins; 118 del; 201 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v9]
> This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Enhancing tests as suggested. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3863/files - new: https://git.openjdk.java.net/jdk/pull/3863/files/80b1392b..79e3621b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3863=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3863=07-08 Stats: 44 lines in 1 file changed: 44 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v8]
On Tue, 1 Jun 2021 07:50:49 GMT, Jan Lahoda wrote: >> This is a preview of a patch implementing JEP 406: Pattern Matching for >> switch (Preview): >> https://bugs.openjdk.java.net/browse/JDK-8213076 >> >> The current draft of the specification is here: >> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html >> >> A summary of notable parts of the patch: >> -to support cases expressions and patterns in cases, there is a new common >> superinterface for expressions and patterns, `CaseLabelTree`, which >> expressions and patterns implement, and a list of case labels is returned >> from `CaseTree.getLabels()`. >> -to support `case default`, there is an implementation of `CaseLabelTree` >> that represents it (`DefaultCaseLabelTree`). It is used also to represent >> the conventional `default` internally and in the newly added methods. >> -in the parser, parenthesized patterns and expressions need to be >> disambiguated when parsing case labels. >> -Lower has been enhanced to handle `case null` for ordinary >> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed >> primitives, as there is no value that is not part of the input domain so >> that could be used to represent `case null`. Requires a bit shuffling with >> values. >> -TransPatterns has been enhanced to handle the pattern matching switch. It >> produces code that delegates to a new bootstrap method, that will classify >> the input value to the switch and return the case number, to which the >> switch then jumps. To support guards, the switches (and the bootstrap >> method) are restartable. The bootstrap method as such is written very simply >> so far, but could be much more optimized later. >> -nullable type patterns are `case String s, null`/`case null, String >> s`/`case null: case String s:`/`case String s: case null:`, handling of >> these required a few tricks in `Attr`, `Flow` and `TransPatterns`. >> >> The specdiff for the change is here (to be updated): >> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Fixing enum switch with patterns with guards. New changes looks good. I suggest to also add tests for strings in switch with guards and numeric with guards, to make sure every kind of switch works well. As discussed offline, we can probably simplify code generation logic for enum switches by leaning on the ConstantBootstraps BSM which allows to create enum constants given a class name and a constant name. - PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v8]
> This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Fixing enum switch with patterns with guards. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3863/files - new: https://git.openjdk.java.net/jdk/pull/3863/files/a49b6109..80b1392b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3863=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3863=06-07 Stats: 82 lines in 3 files changed: 80 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v7]
> This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html Jan Lahoda has updated the pull request incrementally with three additional commits since the last revision: - Fixing tests. - Total pattern dominates the null pattern. - Properly report errors for pattern+default clash. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3863/files - new: https://git.openjdk.java.net/jdk/pull/3863/files/a57d306b..a49b6109 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3863=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3863=05-06 Stats: 52 lines in 6 files changed: 45 ins; 2 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v5]
On Thu, 27 May 2021 10:38:08 GMT, Maurizio Cimadamore
wrote:
>> Jan Lahoda has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 12 commits:
>>
>> - Post-merge fix - need to include jdk.internal.javac in the list of
>> packages used by jdk.compiler again, as we now (again) have a preview
>> feature in javac.
>> - Correcting LineNumberTable for rule switches.
>> - Merging master into JDK-8262891
>> - Fixing various error-related bugs.
>> - Avoiding fall-through from the total case to a synthetic default but
>> changing total patterns to default.
>> - Reflecting recent spec changes.
>> - Reflecting review comments.
>> - Reflecting review comments on SwitchBootstraps.
>> - Trailing whitespaces.
>> - Cleanup, reflecting review comments.
>> - ... and 2 more:
>> https://git.openjdk.java.net/jdk/compare/083416d3...fd748501
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 1657:
>
>> 1655:
>> 1656: try {
>> 1657: boolean enumSwitch = (seltype.tsym.flags() & Flags.ENUM)
>> != 0;
>
> This is getting convoluted enough that an enum on the AST (e.g. SwitchKind)
> might be helpful to save some of these classification properties - which I
> imagine we have to redo at some point later (e.g. in Lower/TransPattern). I'm
> ok with fixing in a followup issue.
Thanks Maurizio. Yes, some of the logic is partly repeated elsewhere, but I
need to investigate how to improve that. I've filled:
https://bugs.openjdk.java.net/browse/JDK-8267929
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v6]
> This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Avoiding unnecessary StackMap point. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3863/files - new: https://git.openjdk.java.net/jdk/pull/3863/files/fd748501..a57d306b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3863=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3863=04-05 Stats: 76 lines in 2 files changed: 72 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v5]
On Thu, 27 May 2021 18:28:13 GMT, Evgeny Mandrikov wrote:
>>> > I've updated the code to produce better/more useful LineNumberTable for
>>> > rule switches.
>>>
>>> @lahodaj I re-tested previous example and tested few others. Now everything
>>> seems to be good with `LineNumberTable` entries +1
>>>
>> [...]
>>> Don't know about importance of this, and whether this was expected, but
>>> decided to mention.
>>
>> Look likes a mistake for me, you only need a StackFrame in front of the call
>> to invokedynamic if it's the target of a goto and in your example, there is
>> no guard so no backward goto.
>
>> in your example, there is no guard so no backward goto
>
> @forax btw this example is not about switch pattern matching - it is about
> already existing string switch, where indy not involed
>
>
> void example(java.lang.String);
> descriptor: (Ljava/lang/String;)V
> flags: (0x)
> Code:
> stack=2, locals=4, args_size=2
> 0: aload_1
> 1: astore_2
> 2: iconst_m1
> 3: istore_3
> 4: aload_2
> 5: invokevirtual #7 // Method
> java/lang/String.hashCode:()I
> 8: lookupswitch { // 1
> 97: 28
> default: 39
> }
> 28: aload_2
> 29: ldc #13 // String a
> 31: invokevirtual #15 // Method
> java/lang/String.equals:(Ljava/lang/Object;)Z
> 34: ifeq 39
> 37: iconst_0
> 38: istore_3
> 39: iload_3
> 40: lookupswitch { // 1
>0: 60
> default: 68
> }
> 60: getstatic #19 // Field
> java/lang/System.out:Ljava/io/PrintStream;
> 63: ldc #13 // String a
> 65: invokevirtual #25 // Method
> java/io/PrintStream.println:(Ljava/lang/String;)V
> 68: return
> LineNumberTable:
> line 3: 0
> line 5: 60
> line 7: 68
> StackMapTable: number_of_entries = 5
> frame_type = 253 /* append */
> offset_delta = 4
> locals = [ class java/lang/String, int ]
> frame_type = 23 /* same */
> frame_type = 10 /* same */
> frame_type = 20 /* same */
> frame_type = 249 /* chop */
> offset_delta = 7
@Godin thanks for noticing the unnecessary point in the StackMap! I've limited
the entry to only be present for pattern matching switches, so ordinary
switches should now look as before. Thanks!
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v5]
On Thu, 27 May 2021 18:19:38 GMT, Rémi Forax wrote:
> in your example, there is no guard so no backward goto
@forax btw this example is not about switch pattern matching - it is about
already existing string switch, where indy not involed
void example(java.lang.String);
descriptor: (Ljava/lang/String;)V
flags: (0x)
Code:
stack=2, locals=4, args_size=2
0: aload_1
1: astore_2
2: iconst_m1
3: istore_3
4: aload_2
5: invokevirtual #7 // Method
java/lang/String.hashCode:()I
8: lookupswitch { // 1
97: 28
default: 39
}
28: aload_2
29: ldc #13 // String a
31: invokevirtual #15 // Method
java/lang/String.equals:(Ljava/lang/Object;)Z
34: ifeq 39
37: iconst_0
38: istore_3
39: iload_3
40: lookupswitch { // 1
0: 60
default: 68
}
60: getstatic #19 // Field
java/lang/System.out:Ljava/io/PrintStream;
63: ldc #13 // String a
65: invokevirtual #25 // Method
java/io/PrintStream.println:(Ljava/lang/String;)V
68: return
LineNumberTable:
line 3: 0
line 5: 60
line 7: 68
StackMapTable: number_of_entries = 5
frame_type = 253 /* append */
offset_delta = 4
locals = [ class java/lang/String, int ]
frame_type = 23 /* same */
frame_type = 10 /* same */
frame_type = 20 /* same */
frame_type = 249 /* chop */
offset_delta = 7
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v5]
On Wed, 26 May 2021 17:52:36 GMT, Jan Lahoda wrote: >> This is a preview of a patch implementing JEP 406: Pattern Matching for >> switch (Preview): >> https://bugs.openjdk.java.net/browse/JDK-8213076 >> >> The current draft of the specification is here: >> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html >> >> A summary of notable parts of the patch: >> -to support cases expressions and patterns in cases, there is a new common >> superinterface for expressions and patterns, `CaseLabelTree`, which >> expressions and patterns implement, and a list of case labels is returned >> from `CaseTree.getLabels()`. >> -to support `case default`, there is an implementation of `CaseLabelTree` >> that represents it (`DefaultCaseLabelTree`). It is used also to represent >> the conventional `default` internally and in the newly added methods. >> -in the parser, parenthesized patterns and expressions need to be >> disambiguated when parsing case labels. >> -Lower has been enhanced to handle `case null` for ordinary >> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed >> primitives, as there is no value that is not part of the input domain so >> that could be used to represent `case null`. Requires a bit shuffling with >> values. >> -TransPatterns has been enhanced to handle the pattern matching switch. It >> produces code that delegates to a new bootstrap method, that will classify >> the input value to the switch and return the case number, to which the >> switch then jumps. To support guards, the switches (and the bootstrap >> method) are restartable. The bootstrap method as such is written very simply >> so far, but could be much more optimized later. >> -nullable type patterns are `case String s, null`/`case null, String >> s`/`case null: case String s:`/`case String s: case null:`, handling of >> these required a few tricks in `Attr`, `Flow` and `TransPatterns`. >> >> The specdiff for the change is here (to be updated): >> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html > > Jan Lahoda has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 12 commits: > > - Post-merge fix - need to include jdk.internal.javac in the list of > packages used by jdk.compiler again, as we now (again) have a preview feature > in javac. > - Correcting LineNumberTable for rule switches. > - Merging master into JDK-8262891 > - Fixing various error-related bugs. > - Avoiding fall-through from the total case to a synthetic default but > changing total patterns to default. > - Reflecting recent spec changes. > - Reflecting review comments. > - Reflecting review comments on SwitchBootstraps. > - Trailing whitespaces. > - Cleanup, reflecting review comments. > - ... and 2 more: > https://git.openjdk.java.net/jdk/compare/083416d3...fd748501 > > I've updated the code to produce better/more useful LineNumberTable for > > rule switches. > > @lahodaj I re-tested previous example and tested few others. Now everything > seems to be good with `LineNumberTable` entries +1 > [...] > Don't know about importance of this, and whether this was expected, but > decided to mention. Look likes a mistake for me, you only need a StackFrame in front of the call to invokedynamic if it's the target of a goto and in your example, there is no guard so no backward goto. - PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v5]
On Wed, 26 May 2021 17:52:36 GMT, Jan Lahoda wrote:
>> This is a preview of a patch implementing JEP 406: Pattern Matching for
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>>
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>>
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common
>> superinterface for expressions and patterns, `CaseLabelTree`, which
>> expressions and patterns implement, and a list of case labels is returned
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree`
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed
>> primitives, as there is no value that is not part of the input domain so
>> that could be used to represent `case null`. Requires a bit shuffling with
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It
>> produces code that delegates to a new bootstrap method, that will classify
>> the input value to the switch and return the case number, to which the
>> switch then jumps. To support guards, the switches (and the bootstrap
>> method) are restartable. The bootstrap method as such is written very simply
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String
>> s`/`case null: case String s:`/`case String s: case null:`, handling of
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>>
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 12 commits:
>
> - Post-merge fix - need to include jdk.internal.javac in the list of
> packages used by jdk.compiler again, as we now (again) have a preview feature
> in javac.
> - Correcting LineNumberTable for rule switches.
> - Merging master into JDK-8262891
> - Fixing various error-related bugs.
> - Avoiding fall-through from the total case to a synthetic default but
> changing total patterns to default.
> - Reflecting recent spec changes.
> - Reflecting review comments.
> - Reflecting review comments on SwitchBootstraps.
> - Trailing whitespaces.
> - Cleanup, reflecting review comments.
> - ... and 2 more:
> https://git.openjdk.java.net/jdk/compare/083416d3...fd748501
> I've updated the code to produce better/more useful LineNumberTable for rule
> switches.
@lahodaj I re-tested previous example and tested few others. Now everything
seems to be good with `LineNumberTable` entries
However I also noticed that for
class Example {
void example(String s) {
switch (s) {
case "a":
System.out.println("a");
}
}
}
there is difference in frames before and after this PR
javap -v -p Example.class
diff is
line 3: 0
line 5: 60
line 7: 68
- StackMapTable: number_of_entries = 4
+ StackMapTable: number_of_entries = 5
frame_type = 253 /* append */
- offset_delta = 28
+ offset_delta = 4
locals = [ class java/lang/String, int ]
+frame_type = 23 /* same */
frame_type = 10 /* same */
frame_type = 20 /* same */
frame_type = 249 /* chop */
and
java -cp asm-util-9.1.jar:asm-9.1.jar org.objectweb.asm.util.Textifier
Example.class
diff is
+ L1
+ FRAME APPEND [java/lang/String I]
ALOAD 2
INVOKEVIRTUAL java/lang/String.hashCode ()I
LOOKUPSWITCH
- 97: L1
- default: L2
- L1
- FRAME APPEND [java/lang/String I]
+ 97: L2
+ default: L3
+ L2
+ FRAME SAME
Don't know about importance of this, and whether this was expected, but decided
to mention.
-
Marked as reviewed by godin (Author).
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v5]
On Wed, 26 May 2021 17:52:36 GMT, Jan Lahoda wrote:
>> This is a preview of a patch implementing JEP 406: Pattern Matching for
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>>
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>>
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common
>> superinterface for expressions and patterns, `CaseLabelTree`, which
>> expressions and patterns implement, and a list of case labels is returned
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree`
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed
>> primitives, as there is no value that is not part of the input domain so
>> that could be used to represent `case null`. Requires a bit shuffling with
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It
>> produces code that delegates to a new bootstrap method, that will classify
>> the input value to the switch and return the case number, to which the
>> switch then jumps. To support guards, the switches (and the bootstrap
>> method) are restartable. The bootstrap method as such is written very simply
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String
>> s`/`case null: case String s:`/`case String s: case null:`, handling of
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>>
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 12 commits:
>
> - Post-merge fix - need to include jdk.internal.javac in the list of
> packages used by jdk.compiler again, as we now (again) have a preview feature
> in javac.
> - Correcting LineNumberTable for rule switches.
> - Merging master into JDK-8262891
> - Fixing various error-related bugs.
> - Avoiding fall-through from the total case to a synthetic default but
> changing total patterns to default.
> - Reflecting recent spec changes.
> - Reflecting review comments.
> - Reflecting review comments on SwitchBootstraps.
> - Trailing whitespaces.
> - Cleanup, reflecting review comments.
> - ... and 2 more:
> https://git.openjdk.java.net/jdk/compare/083416d3...fd748501
Compiler changes look good
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 1657:
> 1655:
> 1656: try {
> 1657: boolean enumSwitch = (seltype.tsym.flags() & Flags.ENUM) !=
> 0;
This is getting convoluted enough that an enum on the AST (e.g. SwitchKind)
might be helpful to save some of these classification properties - which I
imagine we have to redo at some point later (e.g. in Lower/TransPattern). I'm
ok with fixing in a followup issue.
-
Marked as reviewed by mcimadamore (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]
On Tue, 25 May 2021 16:14:56 GMT, Jan Lahoda wrote: > I'd like to note this is a preview feature - we can change the desugaring. At > the same time, I don't think this does not work with sub-patterns (those can > be easily desugared to guards, I think). Yes, but in that case the classcheck du to a sub-pattern matching will be either an instanceof or some other invokedynamic. Having several indy will bloat the code and if we use instanceof, why not using instanceof all along the way. > Regarding efficiency, it may be a balance between classfile overhead (which > will be higher if we need to desugar every guard to a separate method), and > the possibility to tweak the matching at runtime. I fear more the 2 bytes length bytecode limit of a method than the constant pool length limit mostly because people are already using a bunch of lambdas to simulate pattern matching. - PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v5]
On Wed, 26 May 2021 17:52:36 GMT, Jan Lahoda wrote: >> This is a preview of a patch implementing JEP 406: Pattern Matching for >> switch (Preview): >> https://bugs.openjdk.java.net/browse/JDK-8213076 >> >> The current draft of the specification is here: >> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html >> >> A summary of notable parts of the patch: >> -to support cases expressions and patterns in cases, there is a new common >> superinterface for expressions and patterns, `CaseLabelTree`, which >> expressions and patterns implement, and a list of case labels is returned >> from `CaseTree.getLabels()`. >> -to support `case default`, there is an implementation of `CaseLabelTree` >> that represents it (`DefaultCaseLabelTree`). It is used also to represent >> the conventional `default` internally and in the newly added methods. >> -in the parser, parenthesized patterns and expressions need to be >> disambiguated when parsing case labels. >> -Lower has been enhanced to handle `case null` for ordinary >> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed >> primitives, as there is no value that is not part of the input domain so >> that could be used to represent `case null`. Requires a bit shuffling with >> values. >> -TransPatterns has been enhanced to handle the pattern matching switch. It >> produces code that delegates to a new bootstrap method, that will classify >> the input value to the switch and return the case number, to which the >> switch then jumps. To support guards, the switches (and the bootstrap >> method) are restartable. The bootstrap method as such is written very simply >> so far, but could be much more optimized later. >> -nullable type patterns are `case String s, null`/`case null, String >> s`/`case null: case String s:`/`case String s: case null:`, handling of >> these required a few tricks in `Attr`, `Flow` and `TransPatterns`. >> >> The specdiff for the change is here (to be updated): >> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html > > Jan Lahoda has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 12 commits: > > - Post-merge fix - need to include jdk.internal.javac in the list of > packages used by jdk.compiler again, as we now (again) have a preview feature > in javac. > - Correcting LineNumberTable for rule switches. > - Merging master into JDK-8262891 > - Fixing various error-related bugs. > - Avoiding fall-through from the total case to a synthetic default but > changing total patterns to default. > - Reflecting recent spec changes. > - Reflecting review comments. > - Reflecting review comments on SwitchBootstraps. > - Trailing whitespaces. > - Cleanup, reflecting review comments. > - ... and 2 more: > https://git.openjdk.java.net/jdk/compare/083416d3...fd748501 In: https://github.com/openjdk/jdk/pull/3863/commits/7d1abc141ecfecaf0798a2bad899705c116154e7 I've updated the code to produce better/more useful LineNumberTable for rule switches. - PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v5]
> This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits: - Post-merge fix - need to include jdk.internal.javac in the list of packages used by jdk.compiler again, as we now (again) have a preview feature in javac. - Correcting LineNumberTable for rule switches. - Merging master into JDK-8262891 - Fixing various error-related bugs. - Avoiding fall-through from the total case to a synthetic default but changing total patterns to default. - Reflecting recent spec changes. - Reflecting review comments. - Reflecting review comments on SwitchBootstraps. - Trailing whitespaces. - Cleanup, reflecting review comments. - ... and 2 more: https://git.openjdk.java.net/jdk/compare/083416d3...fd748501 - Changes: https://git.openjdk.java.net/jdk/pull/3863/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3863=04 Stats: 4551 lines in 77 files changed: 4235 ins; 119 del; 197 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]
On Tue, 25 May 2021 16:00:43 GMT, Rémi Forax wrote:
> > The reason for this integer (which is not a constant in the case of this
> > switch) is to restart the matching in case guards fail to "match".
> > Considering the example here:
> > ```
> > class Example {
> > void example(Object o) {
> > switch (o) {
> > case String s && s.length() == 0 ->
> > System.out.println("1st case");
> > case String s && s.length() == 1 -> // line 6
> > System.out.println("2nd case"); // line 7
> > case String s -> // line 8
> > System.out.println("3rd case"); // line 9
> > default -> // line 10
> > System.out.println("default case"); // line 11
> > }
> > }
> > }
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > If `o` is `String`, then the first call to indy will be `indy[...](o, 0)`,
> > returning `0`. Then the guard will be evaluated `s.length() == 0`. If the
> > length is not zero, the local variable 3 will be reassigned to `1`(bytecode
> > index 58, 59) and the whole switch is restarted - just this time, the
> > matching in the indy will start at index `1`, not `0`, i.e. `indy[...](o,
> > 1)`. And so on. I believe there is a text explaining the meaning of the
> > parameter in the javadoc of the bootstrap, and in TransPatterns in javac.
>
> The problem with this design is that calling example("foo") forces the VM
> will do 6 checkcast String on "foo", and it doesn't work with sub-patterns.
> Desugaring the guards as static method like with lambdas seems more
> promising, indy can be called with the pairs [String, MethodHandle(s ->
> s.length() == 0)], [String, MethodHandle(s -> s.length() == 0)] and [_,_]
> (with the guards passed as a constant method handles again like lambdas are
> desugared).
> It means that the indy needs to capture all local variables that are used in
> the guards, instead of passing an int.
>
> I believe that a translation using constant method handles for guards is far
> more efficient than using an int and a backward branch but it has a higher
> cost in term of runtime data structures.
I'd like to note this is a preview feature - we can change the desugaring. At
the same time, I don't think this does not work with sub-patterns (those can be
easily desugared to guards, I think). Regarding efficiency, it may be a balance
between classfile overhead (which will be higher if we need to desugar every
guard to a separate method), and the possibility to tweak the matching at
runtime.
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]
On Tue, 25 May 2021 14:22:57 GMT, Jan Lahoda wrote:
> The reason for this integer (which is not a constant in the case of this
> switch) is to restart the matching in case guards fail to "match".
> Considering the example here:
>
> ```
> class Example {
> void example(Object o) {
> switch (o) {
> case String s && s.length() == 0 ->
> System.out.println("1st case");
> case String s && s.length() == 1 -> // line 6
> System.out.println("2nd case"); // line 7
> case String s -> // line 8
> System.out.println("3rd case"); // line 9
> default -> // line 10
> System.out.println("default case"); // line 11
> }
> }
> }
> ```
>
> If `o` is `String`, then the first call to indy will be `indy[...](o, 0)`,
> returning `0`. Then the guard will be evaluated `s.length() == 0`. If the
> length is not zero, the local variable 3 will be reassigned to `1`(bytecode
> index 58, 59) and the whole switch is restarted - just this time, the
> matching in the indy will start at index `1`, not `0`, i.e. `indy[...](o,
> 1)`. And so on. I believe there is a text explaining the meaning of the
> parameter in the javadoc of the bootstrap, and in TransPatterns in javac.
The problem with this design is that calling example("foo") forces the VM will
do 6 checkcast String on "foo", and it doesn't work with sub-patterns.
Desugaring the guards as static method like with lambdas seems more promising,
indy can be called with the pairs [String, MethodHandle(s -> s.length() == 0)],
[String, MethodHandle(s -> s.length() == 0)] and [_,_] (with the guards passed
as a constant method handles again like lambdas are desugared).
It means that the indy needs to capture all local variables that are used in
the guards, instead of passing an int.
I believe that a translation using constant method handles for guards is far
more efficient than using an int and a backward branch but it has a higher cost
in term of runtime data structures.
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]
On Tue, 25 May 2021 14:13:55 GMT, Rémi Forax wrote: > 7: iconst_0 < this zero @forax as far as I understood this will be a value of parameter `startIndex` in `java.lang.runtime. SwitchBootstraps.doSwitch` Please correct me @lahodaj if I'm wrong. - PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]
On Tue, 25 May 2021 14:13:55 GMT, Rémi Forax wrote:
> > Thanks Evgeny, I'll take a look.
> > @forax, do you mean why there is "0" in:
> > 11: invokedynamic #13, 0
> > ?
>
> Not this one, the one on the stack.
>
> 7: iconst_0 < this zero
> 8: istore_3
> 9: aload_2
> 10: iload_3
> 11: invokedynamic #13, 0 // InvokeDynamic
> #0:typeSwitch:(Ljava/lang/Object;I)I
>
> Why the descriptor is (Ljava/lang/Object;I)I instead of (Ljava/lang/Object;)I,
> what is the semantics associated to that integer ?
The reason for this integer (which is not a constant in the case of this
switch) is to restart the matching in case guards fail to "match". Considering
the example here:
class Example {
void example(Object o) {
switch (o) {
case String s && s.length() == 0 ->
System.out.println("1st case");
case String s && s.length() == 1 -> // line 6
System.out.println("2nd case"); // line 7
case String s -> // line 8
System.out.println("3rd case"); // line 9
default -> // line 10
System.out.println("default case"); // line 11
}
}
}
If `o` is `String`, then the first call to indy will be `indy[...](o, 0)`,
returning `0`. Then the guard will be evaluated `s.length() == 0`. If the
length is not zero, the local variable 3 will be reassigned to `1`(bytecode
index 58, 59) and the whole switch is restarted - just this time, the matching
in the indy will start at index `1`, not `0`, i.e. `indy[...](o, 1)`. And so
on. I believe there is a text explaining the meaning of the parameter in the
javadoc of the bootstrap, and in TransPatterns in javac.
>
> > The "0" is an artifact of how invokedynamic is represented in the classfile
> > (invokedynamic, 2 bytes of constant pool reference, byte 0, byte 0) - javap
> > shows the value of the first zero byte. That is probably not needed
> > anymore, but there is nothing special in this patch, I think - all
> > invokedynamic calls look like this, AFAIK.
>
> I know that a little to well, i'm one of the guys behind the design of indy :)
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]
On Tue, 25 May 2021 12:20:02 GMT, Jan Lahoda wrote: > Thanks Evgeny, I'll take a look. > > @forax, do you mean why there is "0" in: > 11: invokedynamic #13, 0 > ? Not this one, the one on the stack. 7: iconst_0 < this zero 8: istore_3 9: aload_2 10: iload_3 11: invokedynamic #13, 0 // InvokeDynamic #0:typeSwitch:(Ljava/lang/Object;I)I Why the descriptor is (Ljava/lang/Object;I)I instead of (Ljava/lang/Object;)I, what is the semantics associated to that integer ? > The "0" is an artifact of how invokedynamic is represented in the classfile > (invokedynamic, 2 bytes of constant pool reference, byte 0, byte 0) - javap > shows the value of the first zero byte. That is probably not needed anymore, > but there is nothing special in this patch, I think - all invokedynamic calls > look like this, AFAIK. I know that a little to well, i'm one of the guys behind the design of indy :) - PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]
On Wed, 19 May 2021 08:00:12 GMT, Jan Lahoda wrote: >> This is a preview of a patch implementing JEP 406: Pattern Matching for >> switch (Preview): >> https://bugs.openjdk.java.net/browse/JDK-8213076 >> >> The current draft of the specification is here: >> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html >> >> A summary of notable parts of the patch: >> -to support cases expressions and patterns in cases, there is a new common >> superinterface for expressions and patterns, `CaseLabelTree`, which >> expressions and patterns implement, and a list of case labels is returned >> from `CaseTree.getLabels()`. >> -to support `case default`, there is an implementation of `CaseLabelTree` >> that represents it (`DefaultCaseLabelTree`). It is used also to represent >> the conventional `default` internally and in the newly added methods. >> -in the parser, parenthesized patterns and expressions need to be >> disambiguated when parsing case labels. >> -Lower has been enhanced to handle `case null` for ordinary >> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed >> primitives, as there is no value that is not part of the input domain so >> that could be used to represent `case null`. Requires a bit shuffling with >> values. >> -TransPatterns has been enhanced to handle the pattern matching switch. It >> produces code that delegates to a new bootstrap method, that will classify >> the input value to the switch and return the case number, to which the >> switch then jumps. To support guards, the switches (and the bootstrap >> method) are restartable. The bootstrap method as such is written very simply >> so far, but could be much more optimized later. >> -nullable type patterns are `case String s, null`/`case null, String >> s`/`case null: case String s:`/`case String s: case null:`, handling of >> these required a few tricks in `Attr`, `Flow` and `TransPatterns`. >> >> The specdiff for the change is here (to be updated): >> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Fixing various error-related bugs. Thanks Evgeny, I'll take a look. @forax, do you mean why there is "0" in: 11: invokedynamic #13, 0 ? The "0" is an artifact of how invokedynamic is represented in the classfile (invokedynamic, 2 bytes of constant pool reference, byte 0, byte 0) - javap shows the value of the first zero byte. That is probably not needed anymore, but there is nothing special in this patch, I think - all invokedynamic calls look like this, AFAIK. - PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]
- Mail original -
> De: "Evgeny Mandrikov"
> À: "build-dev" , "compiler-dev"
> , "core-libs-dev"
> , "javadoc-dev"
> Envoyé: Mardi 25 Mai 2021 11:32:03
> Objet: Re: RFR: 8262891: Compiler implementation for Pattern Matching for
> switch (Preview) [v4]
> On Mon, 17 May 2021 19:01:01 GMT, Jan Lahoda wrote:
>
>>> Good work. There's a lot to take in here. I think overall, it holds up well
>>> - I
>>> like how case labels have been extended to accommodate for patterns. In the
>>> front-end, I think there are some questions over the split between Attr and
>>> Flow - maybe it is unavoidable, but I'm not sure why some control-flow
>>> analysis
>>> happens in Attr instead of Flow and I left some questions to make sure I
>>> understand what's happening.
>>>
>>> In the backend it's mostly good work - but overall the structure of the
>>> code,
>>> both in Lower and in TransPattern is getting very difficult to follow, given
>>> there are many different kind of switches each with its own little twist
>>> attached to it. It would be nice, I think (maybe in a separate cleanup?) if
>>> the
>>> code paths serving the different switch kinds could be made more separate,
>>> so
>>> that, when reading the code we can worry only about one possible code shape.
>>> That would make the code easier to document as well.
>>
>> @mcimadamore, @forax, thanks a lot for comments! I tried to apply the
>> requested
>> changes in recent commits
>> (https://github.com/openjdk/jdk/pull/3863/commits/3fc2502a33cec20f39fe571eb25538ee3b459a9b
>> https://github.com/openjdk/jdk/pull/3863/commits/aeddb85e65bf77ed62dc7fa1ad00c29646d02c99
>> ).
>>
>> I've also tried to update the implementation for the latest spec changes
>> here:
>> https://github.com/openjdk/jdk/pull/3863/commits/54ba974e1aac00bbde1c3bd2627f01caaaeda09b
>>
>> The spec changes are: total patterns are nullable; pattern matching ("new")
>> statement switches must be complete (as switch expressions).
>>
>> Any further feedback is welcome!
>
> Hi @lahodaj ,
>
> I tried `javac` built from this PR and for the following `Example.java`
>
>
> class Example {
>void example(Object o) {
>switch (o) {
>case String s && s.length() == 0 ->
>System.out.println("1st case");
>case String s && s.length() == 1 -> // line 6
>System.out.println("2nd case"); // line 7
>case String s -> // line 8
>System.out.println("3rd case"); // line 9
>default -> // line 10
>System.out.println("default case"); // line 11
>}
>}
> }
>
>
> execution of
>
>
> javac --enable-preview --release 17 Example.java
> javap -v -p Example.class
>
>
> produces
>
>
> void example(java.lang.Object);
>descriptor: (Ljava/lang/Object;)V
>flags: (0x)
>Code:
> stack=2, locals=7, args_size=2
> 0: aload_1
> 1: dup
> 2: invokestatic #7 // Method
>
> java/util/Objects.requireNonNull:(Ljava/lang/Object;)Ljava/lang/Object;
> 5: pop
> 6: astore_2
> 7: iconst_0
> 8: istore_3
> 9: aload_2
>10: iload_3
>11: invokedynamic #13, 0 // InvokeDynamic
>#0:typeSwitch:(Ljava/lang/Object;I)I
>16: tableswitch { // 0 to 2
> 0: 44
> 1: 74
> 2: 105
> default: 122
>}
>44: aload_2
>45: checkcast #17 // class java/lang/String
>48: astore4
>50: aload 4
>52: invokevirtual #19 // Method
> java/lang/String.length:()I
>55: ifeq 63
>58: iconst_1
>59: istore_3
>60: goto 9
>63: getstatic #23 // Field
>java/lang/System.out:Ljava/io/PrintStream;
>66: ldc #29 // String 1st case
>68: invokevirtual #31 // Method
>java/io/PrintStream.println:(Ljava/lang/String;)V
>71: goto 133
>74: aload_2
>75: checkca
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]
On Mon, 17 May 2021 19:01:01 GMT, Jan Lahoda wrote:
>> Good work. There's a lot to take in here. I think overall, it holds up well
>> - I like how case labels have been extended to accommodate for patterns. In
>> the front-end, I think there are some questions over the split between Attr
>> and Flow - maybe it is unavoidable, but I'm not sure why some control-flow
>> analysis happens in Attr instead of Flow and I left some questions to make
>> sure I understand what's happening.
>>
>> In the backend it's mostly good work - but overall the structure of the
>> code, both in Lower and in TransPattern is getting very difficult to follow,
>> given there are many different kind of switches each with its own little
>> twist attached to it. It would be nice, I think (maybe in a separate
>> cleanup?) if the code paths serving the different switch kinds could be made
>> more separate, so that, when reading the code we can worry only about one
>> possible code shape. That would make the code easier to document as well.
>
> @mcimadamore, @forax, thanks a lot for comments! I tried to apply the
> requested changes in recent commits
> (https://github.com/openjdk/jdk/pull/3863/commits/3fc2502a33cec20f39fe571eb25538ee3b459a9b
>
> https://github.com/openjdk/jdk/pull/3863/commits/aeddb85e65bf77ed62dc7fa1ad00c29646d02c99
> ).
>
> I've also tried to update the implementation for the latest spec changes here:
> https://github.com/openjdk/jdk/pull/3863/commits/54ba974e1aac00bbde1c3bd2627f01caaaeda09b
>
> The spec changes are: total patterns are nullable; pattern matching ("new")
> statement switches must be complete (as switch expressions).
>
> Any further feedback is welcome!
Hi @lahodaj ,
I tried `javac` built from this PR and for the following `Example.java`
class Example {
void example(Object o) {
switch (o) {
case String s && s.length() == 0 ->
System.out.println("1st case");
case String s && s.length() == 1 -> // line 6
System.out.println("2nd case"); // line 7
case String s -> // line 8
System.out.println("3rd case"); // line 9
default -> // line 10
System.out.println("default case"); // line 11
}
}
}
execution of
javac --enable-preview --release 17 Example.java
javap -v -p Example.class
produces
void example(java.lang.Object);
descriptor: (Ljava/lang/Object;)V
flags: (0x)
Code:
stack=2, locals=7, args_size=2
0: aload_1
1: dup
2: invokestatic #7 // Method
java/util/Objects.requireNonNull:(Ljava/lang/Object;)Ljava/lang/Object;
5: pop
6: astore_2
7: iconst_0
8: istore_3
9: aload_2
10: iload_3
11: invokedynamic #13, 0 // InvokeDynamic
#0:typeSwitch:(Ljava/lang/Object;I)I
16: tableswitch { // 0 to 2
0: 44
1: 74
2: 105
default: 122
}
44: aload_2
45: checkcast #17 // class java/lang/String
48: astore4
50: aload 4
52: invokevirtual #19 // Method
java/lang/String.length:()I
55: ifeq 63
58: iconst_1
59: istore_3
60: goto 9
63: getstatic #23 // Field
java/lang/System.out:Ljava/io/PrintStream;
66: ldc #29 // String 1st case
68: invokevirtual #31 // Method
java/io/PrintStream.println:(Ljava/lang/String;)V
71: goto 133
74: aload_2
75: checkcast #17 // class java/lang/String
78: astore5
80: aload 5
82: invokevirtual #19 // Method
java/lang/String.length:()I
85: iconst_1
86: if_icmpeq 94
89: iconst_2
90: istore_3
91: goto 9
94: getstatic #23 // Field
java/lang/System.out:Ljava/io/PrintStream;
97: ldc #37 // String 2nd case
99: invokevirtual #31 // Method
java/io/PrintStream.println:(Ljava/lang/String;)V
102: goto 133
105: aload_2
106: checkcast #17 // class java/lang/String
109: astore6
111: getstatic #23 // Field
java/lang/System.out:Ljava/io/PrintStream;
114: ldc #39 // String 3rd case
116: invokevirtual #31 // Method
java/io/PrintStream.println:(Ljava/lang/String;)V
119: goto 133
122: getstatic #23 //
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]
> This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Fixing various error-related bugs. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3863/files - new: https://git.openjdk.java.net/jdk/pull/3863/files/5fa8005a..0875377b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3863=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3863=02-03 Stats: 117 lines in 6 files changed: 94 ins; 13 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v3]
> This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Avoiding fall-through from the total case to a synthetic default but changing total patterns to default. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3863/files - new: https://git.openjdk.java.net/jdk/pull/3863/files/54ba974e..5fa8005a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3863=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3863=01-02 Stats: 22 lines in 2 files changed: 21 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v2]
On Mon, 17 May 2021 19:04:11 GMT, Jan Lahoda wrote: >> This is a preview of a patch implementing JEP 406: Pattern Matching for >> switch (Preview): >> https://bugs.openjdk.java.net/browse/JDK-8213076 >> >> The current draft of the specification is here: >> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html >> >> A summary of notable parts of the patch: >> -to support cases expressions and patterns in cases, there is a new common >> superinterface for expressions and patterns, `CaseLabelTree`, which >> expressions and patterns implement, and a list of case labels is returned >> from `CaseTree.getLabels()`. >> -to support `case default`, there is an implementation of `CaseLabelTree` >> that represents it (`DefaultCaseLabelTree`). It is used also to represent >> the conventional `default` internally and in the newly added methods. >> -in the parser, parenthesized patterns and expressions need to be >> disambiguated when parsing case labels. >> -Lower has been enhanced to handle `case null` for ordinary >> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed >> primitives, as there is no value that is not part of the input domain so >> that could be used to represent `case null`. Requires a bit shuffling with >> values. >> -TransPatterns has been enhanced to handle the pattern matching switch. It >> produces code that delegates to a new bootstrap method, that will classify >> the input value to the switch and return the case number, to which the >> switch then jumps. To support guards, the switches (and the bootstrap >> method) are restartable. The bootstrap method as such is written very simply >> so far, but could be much more optimized later. >> -nullable type patterns are `case String s, null`/`case null, String >> s`/`case null: case String s:`/`case String s: case null:`, handling of >> these required a few tricks in `Attr`, `Flow` and `TransPatterns`. >> >> The specdiff for the change is here (to be updated): >> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html > > Jan Lahoda has updated the pull request incrementally with three additional > commits since the last revision: > > - Reflecting recent spec changes. > - Reflecting review comments. > - Reflecting review comments on SwitchBootstraps. It's not clear to me if it's the first change and several will follow or not. The code looks good but the metaprotocol is not the right one IMO, i would prefer the one we discuss in April https://mail.openjdk.java.net/pipermail/amber-spec-experts/2021-April/002992.html But this can be tackle in another PR - Marked as reviewed by fo...@github.com (no known OpenJDK username). PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v2]
> This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html Jan Lahoda has updated the pull request incrementally with three additional commits since the last revision: - Reflecting recent spec changes. - Reflecting review comments. - Reflecting review comments on SwitchBootstraps. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3863/files - new: https://git.openjdk.java.net/jdk/pull/3863/files/5e663d70..54ba974e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3863=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3863=00-01 Stats: 544 lines in 18 files changed: 431 ins; 68 del; 45 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v2]
On Tue, 4 May 2021 20:48:34 GMT, Maurizio Cimadamore
wrote:
>> Jan Lahoda has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - Reflecting recent spec changes.
>> - Reflecting review comments.
>> - Reflecting review comments on SwitchBootstraps.
>
> Good work. There's a lot to take in here. I think overall, it holds up well -
> I like how case labels have been extended to accommodate for patterns. In the
> front-end, I think there are some questions over the split between Attr and
> Flow - maybe it is unavoidable, but I'm not sure why some control-flow
> analysis happens in Attr instead of Flow and I left some questions to make
> sure I understand what's happening.
>
> In the backend it's mostly good work - but overall the structure of the code,
> both in Lower and in TransPattern is getting very difficult to follow, given
> there are many different kind of switches each with its own little twist
> attached to it. It would be nice, I think (maybe in a separate cleanup?) if
> the code paths serving the different switch kinds could be made more
> separate, so that, when reading the code we can worry only about one possible
> code shape. That would make the code easier to document as well.
@mcimadamore, @forax, thanks a lot for comments! I tried to apply the requested
changes in recent commits
(https://github.com/openjdk/jdk/pull/3863/commits/3fc2502a33cec20f39fe571eb25538ee3b459a9b
https://github.com/openjdk/jdk/pull/3863/commits/aeddb85e65bf77ed62dc7fa1ad00c29646d02c99
).
I've also tried to update the implementation for the latest spec changes here:
https://github.com/openjdk/jdk/pull/3863/commits/54ba974e1aac00bbde1c3bd2627f01caaaeda09b
The spec changes are: total patterns are nullable; pattern matching ("new")
statement switches must be complete (as switch expressions).
Any further feedback is welcome!
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview)
On Tue, 4 May 2021 16:41:44 GMT, Jan Lahoda wrote:
> This is a preview of a patch implementing JEP 406: Pattern Matching for
> switch (Preview):
> https://bugs.openjdk.java.net/browse/JDK-8213076
>
> The current draft of the specification is here:
> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>
> A summary of notable parts of the patch:
> -to support cases expressions and patterns in cases, there is a new common
> superinterface for expressions and patterns, `CaseLabelTree`, which
> expressions and patterns implement, and a list of case labels is returned
> from `CaseTree.getLabels()`.
> -to support `case default`, there is an implementation of `CaseLabelTree`
> that represents it (`DefaultCaseLabelTree`). It is used also to represent the
> conventional `default` internally and in the newly added methods.
> -in the parser, parenthesized patterns and expressions need to be
> disambiguated when parsing case labels.
> -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive,
> String, enum) switches. This is a bit tricky for boxed primitives, as there
> is no value that is not part of the input domain so that could be used to
> represent `case null`. Requires a bit shuffling with values.
> -TransPatterns has been enhanced to handle the pattern matching switch. It
> produces code that delegates to a new bootstrap method, that will classify
> the input value to the switch and return the case number, to which the switch
> then jumps. To support guards, the switches (and the bootstrap method) are
> restartable. The bootstrap method as such is written very simply so far, but
> could be much more optimized later.
> -nullable type patterns are `case String s, null`/`case null, String s`/`case
> null: case String s:`/`case String s: case null:`, handling of these required
> a few tricks in `Attr`, `Flow` and `TransPatterns`.
>
> The specdiff for the change is here (to be updated):
> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 127:
> 125: Stream.of(labels).forEach(SwitchBootstraps::verifyLabel);
> 126:
> 127: return new TypeSwitchCallSite(invocationType, labels);
See why below
MethodHandle target = MethodHandles.insertArguments(DO_SWITCH, 2, labels);
return new ConstantCallsite(target);
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 134:
> 132: throw new IllegalArgumentException("null label found");
> 133: }
> 134: if (label.getClass() != Class.class &&
store `label.getClass` in a local variable,
it's too bad that you can no use pattern matching here :)
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 141:
> 139: }
> 140:
> 141: static class TypeSwitchCallSite extends ConstantCallSite {
That's weird, having a callsite extending MutableCallSite is expected but
having a callsite extending constant callsite is useless because you can not
change it after being constructed.
As an interesting trivia, the VM does not store the CallSite returned by the
BSM, but only the target inside it.
So there is no point of keeping a ConstantCallSite around.
So `doSwitch()` should be static and takes the array of Object[] as parameter,
will array will be injected with an insertArguments().
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 157:
> 155: Class targetClass = target.getClass();
> 156: for (int i = startIndex; i < labels.length; i++) {
> 157: if (labels[i] instanceof Class) {
label[i] should be stored is a local variable and
using `instanceof Class c` (like the other instanceof below) will make the
code more readable
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview)
On Tue, 4 May 2021 16:41:44 GMT, Jan Lahoda wrote:
> This is a preview of a patch implementing JEP 406: Pattern Matching for
> switch (Preview):
> https://bugs.openjdk.java.net/browse/JDK-8213076
>
> The current draft of the specification is here:
> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>
> A summary of notable parts of the patch:
> -to support cases expressions and patterns in cases, there is a new common
> superinterface for expressions and patterns, `CaseLabelTree`, which
> expressions and patterns implement, and a list of case labels is returned
> from `CaseTree.getLabels()`.
> -to support `case default`, there is an implementation of `CaseLabelTree`
> that represents it (`DefaultCaseLabelTree`). It is used also to represent the
> conventional `default` internally and in the newly added methods.
> -in the parser, parenthesized patterns and expressions need to be
> disambiguated when parsing case labels.
> -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive,
> String, enum) switches. This is a bit tricky for boxed primitives, as there
> is no value that is not part of the input domain so that could be used to
> represent `case null`. Requires a bit shuffling with values.
> -TransPatterns has been enhanced to handle the pattern matching switch. It
> produces code that delegates to a new bootstrap method, that will classify
> the input value to the switch and return the case number, to which the switch
> then jumps. To support guards, the switches (and the bootstrap method) are
> restartable. The bootstrap method as such is written very simply so far, but
> could be much more optimized later.
> -nullable type patterns are `case String s, null`/`case null, String s`/`case
> null: case String s:`/`case String s: case null:`, handling of these required
> a few tricks in `Attr`, `Flow` and `TransPatterns`.
>
> The specdiff for the change is here (to be updated):
> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 77:
> 75: }
> 76:
> 77: private static MethodHandle typeInitHook(T
> receiver) {
There is no point to have a type parameter here,
private static MethodHandle typeInitHook(CallSite receiver) {
will work the same
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 131:
> 129:
> 130: private static void verifyLabel(Object label) {
> 131: if (Objects.isNull(label)) {
`if (label == true) {` is more readable as said in the javadoc of
`Objects.isNull`
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview)
On Tue, 4 May 2021 16:41:44 GMT, Jan Lahoda wrote: > This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html make/CompileInterimLangtools.gmk line 52: > 50: > 51: $(eval $(call SetupCopyFiles, COPY_PREVIEW_FEATURES, \ > 52: FILES := > $(TOPDIR)/src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java > $(TOPDIR)/src/java.base/share/classes/jdk/internal/javac/NoPreview.java, \ Please break this line (adding 4 additional space indent from the original line). Otherwise build changes ok. - PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview)
On Tue, 4 May 2021 20:35:45 GMT, Maurizio Cimadamore
wrote:
>> This is a preview of a patch implementing JEP 406: Pattern Matching for
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>>
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>>
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common
>> superinterface for expressions and patterns, `CaseLabelTree`, which
>> expressions and patterns implement, and a list of case labels is returned
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree`
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed
>> primitives, as there is no value that is not part of the input domain so
>> that could be used to represent `case null`. Requires a bit shuffling with
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It
>> produces code that delegates to a new bootstrap method, that will classify
>> the input value to the switch and return the case number, to which the
>> switch then jumps. To support guards, the switches (and the bootstrap
>> method) are restartable. The bootstrap method as such is written very simply
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String
>> s`/`case null: case String s:`/`case String s: case null:`, handling of
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>>
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 102:
>
>> 100: * @param labels non-null case labels - {@code String} and {@code
>> Integer} constants
>> 101: *and {@code Class} instances
>> 102: * @return the index into {@code labels} of the target value, if
>> the target
>
> Doesn't return an index. Returns a CallSite which, when invoked with an
> argument of type E (where E is the type of the target expression), returns
> the index into...
It should also mention that the handle returned accepts a start index (which is
used by the continue logic)
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview)
On Tue, 4 May 2021 20:48:34 GMT, Maurizio Cimadamore
wrote:
>> This is a preview of a patch implementing JEP 406: Pattern Matching for
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>>
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>>
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common
>> superinterface for expressions and patterns, `CaseLabelTree`, which
>> expressions and patterns implement, and a list of case labels is returned
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree`
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed
>> primitives, as there is no value that is not part of the input domain so
>> that could be used to represent `case null`. Requires a bit shuffling with
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It
>> produces code that delegates to a new bootstrap method, that will classify
>> the input value to the switch and return the case number, to which the
>> switch then jumps. To support guards, the switches (and the bootstrap
>> method) are restartable. The bootstrap method as such is written very simply
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String
>> s`/`case null: case String s:`/`case String s: case null:`, handling of
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>>
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Good work. There's a lot to take in here. I think overall, it holds up well -
> I like how case labels have been extended to accommodate for patterns. In the
> front-end, I think there are some questions over the split between Attr and
> Flow - maybe it is unavoidable, but I'm not sure why some control-flow
> analysis happens in Attr instead of Flow and I left some questions to make
> sure I understand what's happening.
>
> In the backend it's mostly good work - but overall the structure of the code,
> both in Lower and in TransPattern is getting very difficult to follow, given
> there are many different kind of switches each with its own little twist
> attached to it. It would be nice, I think (maybe in a separate cleanup?) if
> the code paths serving the different switch kinds could be made more
> separate, so that, when reading the code we can worry only about one possible
> code shape. That would make the code easier to document as well.
@mcimadamore, thanks a lot for the comments! I tried to reflect most of them in
https://github.com/openjdk/jdk/pull/3863/commits/1a5a424139a52d0f93e16980c6b42cf29dd908ef
- please let me know how that looks. Thanks!
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 1790:
>
>> 1788: if (isTotal) {
>> 1789: if (hasTotalPattern) {
>> 1790: log.error(pat.pos(),
>> Errors.DuplicateTotalPattern);
>
> Hard to explain - but a lot of the code here feels more like it belongs to
> `Flow` than to `Attr`. E.g. these "duplicateXYZ" labels really want to say
> "unreachable code", I think. Is there a strong reason as to why all this code
> shouldn't (apart from code computing bindings) move to Flow? Seems that the
> logic is partially duplicated anyway...
`Attr` was always checking duplicated constants and duplicated default, so
checking for pattern dominance (which could be seen as an extension to
duplicate constant labels) and total patterns (which is an extension to
duplicated default) here seemed reasonable. We also have a couple of other
similar checks performed by `Attr`, like duplicated statement labels, or that
exception types in multicatch are disjoint. These are checks that don't need
DA/DU, while I think `Flow` does mostly checks that require DA/DU.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java line 3649:
>
>> 3647: if (!enumSwitch && !stringSwitch &&
>> !selector.type.isPrimitive() &&
>> 3648: cases.stream().anyMatch(c ->
>> TreeInfo.isNull(c.labels.head))) {
>> 3649: //a switch over a boxed primitive, with a null case. Pick
>> two constants that are
>
> Is this comment correct? If we're here, can't this be just any pattern switch?
Patterns switches are resolved before Lower, so in Lower
RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview)
This is a preview of a patch implementing JEP 406: Pattern Matching for switch (Preview): https://bugs.openjdk.java.net/browse/JDK-8213076 The current draft of the specification is here: http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html A summary of notable parts of the patch: -to support cases expressions and patterns in cases, there is a new common superinterface for expressions and patterns, `CaseLabelTree`, which expressions and patterns implement, and a list of case labels is returned from `CaseTree.getLabels()`. -to support `case default`, there is an implementation of `CaseLabelTree` that represents it (`DefaultCaseLabelTree`). It is used also to represent the conventional `default` internally and in the newly added methods. -in the parser, parenthesized patterns and expressions need to be disambiguated when parsing case labels. -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, String, enum) switches. This is a bit tricky for boxed primitives, as there is no value that is not part of the input domain so that could be used to represent `case null`. Requires a bit shuffling with values. -TransPatterns has been enhanced to handle the pattern matching switch. It produces code that delegates to a new bootstrap method, that will classify the input value to the switch and return the case number, to which the switch then jumps. To support guards, the switches (and the bootstrap method) are restartable. The bootstrap method as such is written very simply so far, but could be much more optimized later. -nullable type patterns are `case String s, null`/`case null, String s`/`case null: case String s:`/`case String s: case null:`, handling of these required a few tricks in `Attr`, `Flow` and `TransPatterns`. The specdiff for the change is here (to be updated): http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html - Commit messages: - Trailing whitespaces. - Cleanup, reflecting review comments. - Merge branch 'master' into JDK-8262891 - JDK-8262891: Compiler implementation for Pattern Matching for switch Changes: https://git.openjdk.java.net/jdk/pull/3863/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3863=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8262891 Stats: 3927 lines in 70 files changed: 3636 ins; 97 del; 194 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview)
On Tue, 4 May 2021 16:41:44 GMT, Jan Lahoda wrote:
> This is a preview of a patch implementing JEP 406: Pattern Matching for
> switch (Preview):
> https://bugs.openjdk.java.net/browse/JDK-8213076
>
> The current draft of the specification is here:
> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>
> A summary of notable parts of the patch:
> -to support cases expressions and patterns in cases, there is a new common
> superinterface for expressions and patterns, `CaseLabelTree`, which
> expressions and patterns implement, and a list of case labels is returned
> from `CaseTree.getLabels()`.
> -to support `case default`, there is an implementation of `CaseLabelTree`
> that represents it (`DefaultCaseLabelTree`). It is used also to represent the
> conventional `default` internally and in the newly added methods.
> -in the parser, parenthesized patterns and expressions need to be
> disambiguated when parsing case labels.
> -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive,
> String, enum) switches. This is a bit tricky for boxed primitives, as there
> is no value that is not part of the input domain so that could be used to
> represent `case null`. Requires a bit shuffling with values.
> -TransPatterns has been enhanced to handle the pattern matching switch. It
> produces code that delegates to a new bootstrap method, that will classify
> the input value to the switch and return the case number, to which the switch
> then jumps. To support guards, the switches (and the bootstrap method) are
> restartable. The bootstrap method as such is written very simply so far, but
> could be much more optimized later.
> -nullable type patterns are `case String s, null`/`case null, String s`/`case
> null: case String s:`/`case String s: case null:`, handling of these required
> a few tricks in `Attr`, `Flow` and `TransPatterns`.
>
> The specdiff for the change is here (to be updated):
> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
Good work. There's a lot to take in here. I think overall, it holds up well - I
like how case labels have been extended to accommodate for patterns. In the
front-end, I think there are some questions over the split between Attr and
Flow - maybe it is unavoidable, but I'm not sure why some control-flow analysis
happens in Attr instead of Flow and I left some questions to make sure I
understand what's happening.
In the backend it's mostly good work - but overall the structure of the code,
both in Lower and in TransPattern is getting very difficult to follow, given
there are many different kind of switches each with its own little twist
attached to it. It would be nice, I think (maybe in a separate cleanup?) if the
code paths serving the different switch kinds could be made more separate, so
that, when reading the code we can worry only about one possible code shape.
That would make the code easier to document as well.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 100:
> 98: * the {@code NameAndType} of the {@code
> InvokeDynamic}
> 99: * structure and is stacked automatically by
> the VM.
> 100: * @param labels non-null case labels - {@code String} and {@code
> Integer} constants
Can these types be mixed? E.g. can I pass, as static args: `42`,
`Runnable.class`, `"hello"` ? If not, should be document, and throw?
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 102:
> 100: * @param labels non-null case labels - {@code String} and {@code
> Integer} constants
> 101: *and {@code Class} instances
> 102: * @return the index into {@code labels} of the target value, if the
> target
Doesn't return an index. Returns a CallSite which, when invoked with an
argument of type E (where E is the type of the target expression), returns the
index into...
src/jdk.compiler/share/classes/com/sun/source/tree/TreeVisitor.java line 306:
> 304:
> 305: /**
> 306: * Visits an GuardPatternTree node.
Suggestion:
* Visits a GuardPatternTree node.
src/jdk.compiler/share/classes/com/sun/source/tree/TreeVisitor.java line 316:
> 314:
> 315: /**
> 316: * Visits an AndPatternTree node.
Is this comment correct?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 1685:
> 1683: log.error(DiagnosticFlag.SOURCE_LEVEL,
> selector.pos(),
> 1684:
> Feature.PATTERN_SWITCH.error(this.sourceName));
> 1685: allowPatternSwitch = true;
I assume this logic is to show only one error in case we're compiling multiple
methods with pattern switches and preview features are not enabled? Is this
consistent with what happens with other preview features though?
