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) [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 👍
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) [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 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 //
