8258422: Cleanup unnecessary null comparison before instanceof check in java.base #20
Conversation
|
Hi @turbanoff, welcome to this OpenJDK project and thanks for contributing! We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user turbanoff" as summary for the issue. If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing |
|
@turbanoff The following labels will be automatically applied to this pull request: |
openjdk
bot
added
nio
|
/signed |
|
Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated! |
turbanoff
force-pushed
the
cleanup_null_checks_before_instanceof_in_java.base
branch
from
71fa52f to
f09bbd2
Compare
bridgekeeper
bot
removed
oca
|
@turbanoff This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
I believe this changes is useful and still actual:
|
|
I’ll see if I can get somebody to take a look at this. |
|
This seems like a reasonable change, which improves readability. My preference is to wait a little longer (hopefully no more than a couple of weeks), until JEP 394 - "Pattern Matching for instanceof" is finalised, then we can remove the explicit casts in many of these cases. For example: |
|
Related issue - https://bugs.openjdk.java.net/browse/JDK-8257448 |
|
Hi @turbanoff, I've logged a JBS issue for tracking this change: JEP 394 is finalized now, so I guess you could follow-up Chris suggestion to remove the explicit casts. After the fix is properly reviewed and marked as ready for the integration (you'll need to issue With Best Regards, |
|
/issue JDK-8258422 |
|
@AlekseiEfimov Only the author (@turbanoff) is allowed to issue the |
|
/issue JDK-8258422 |
openjdk
bot
changed the title
|
@turbanoff The primary solved issue for a PR is set through the PR title. Since the current title does not contain an issue reference, it will now be updated. |
Webrevs
|
…in java.base use instanceof pattern matching
turbanoff
force-pushed
the
cleanup_null_checks_before_instanceof_in_java.base
branch
from
f09bbd2 to
603cd36
Compare
…in java.base use instanceof pattern matching in UnixPath too
|
@AlekseiEfimov The command |
21 similar comments
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
|
@AlekseiEfimov The command |
Corresponding pull request in checker-framework: typetools/checker-framework#2882
turbanoff
deleted the
cleanup_null_checks_before_instanceof_in_java.base
branch
After JDK-8283091, the loop below can be vectorized partially.
Statement 1 can be vectorized but statement 2 can't.
```
// int[] iArr; long[] lArrFld; int i1,i2;
for (i1 = 6; i1 < 227; i1++) {
iArr[i1] += lArrFld[i1]++; // statement 1
iArr[i1 + 1] -= (i2++); // statement 2
}
```
But we got incorrect results because the vector packs of iArr are
scheduled incorrectly like:
```
...
load_vector XMM1,[R8 + openjdk#16 + R11 << openjdk#2]
movl RDI, [R8 + openjdk#20 + R11 << openjdk#2] # int
load_vector XMM2,[R9 + openjdk#8 + R11 << openjdk#3]
subl RDI, R11 # int
vpaddq XMM3,XMM2,XMM0 ! add packedL
store_vector [R9 + openjdk#8 + R11 << openjdk#3],XMM3
vector_cast_l2x XMM2,XMM2 !
vpaddd XMM1,XMM2,XMM1 ! add packedI
addl RDI, openjdk#228 # int
movl [R8 + openjdk#20 + R11 << openjdk#2], RDI # int
movl RBX, [R8 + openjdk#24 + R11 << openjdk#2] # int
subl RBX, R11 # int
addl RBX, openjdk#227 # int
movl [R8 + openjdk#24 + R11 << openjdk#2], RBX # int
...
movl RBX, [R8 + openjdk#40 + R11 << openjdk#2] # int
subl RBX, R11 # int
addl RBX, openjdk#223 # int
movl [R8 + openjdk#40 + R11 << openjdk#2], RBX # int
movl RDI, [R8 + openjdk#44 + R11 << openjdk#2] # int
subl RDI, R11 # int
addl RDI, openjdk#222 # int
movl [R8 + openjdk#44 + R11 << openjdk#2], RDI # int
store_vector [R8 + openjdk#16 + R11 << openjdk#2],XMM1
...
```
simplified as:
```
load_vector iArr in statement 1
unvectorized loads/stores in statement 2
store_vector iArr in statement 1
```
We cannot pick the memory state from the first load for LoadI pack
here, as the LoadI vector operation must load the new values in memory
after iArr writes 'iArr[i1 + 1] - (i2++)' to 'iArr[i1 + 1]'(statement 2).
We must take the memory state of the last load where we have assigned
new values ('iArr[i1 + 1] - (i2++)') to the iArr array.
In JDK-8240281, we picked the memory state of the first load. Different
from the scenario in JDK-8240281, the store, which is dependent on an
earlier load here, is in a pack to be scheduled and the LoadI pack
depends on the last_mem. As designed[2], to schedule the StoreI pack,
all memory operations in another single pack should be moved in the same
direction. We know that the store in the pack depends on one of loads in
the LoadI pack, so the LoadI pack should be scheduled before the StoreI
pack. And the LoadI pack depends on the last_mem, so the last_mem must
be scheduled before the LoadI pack and also before the store pack.
Therefore, we need to take the memory state of the last load for the
LoadI pack here.
To fix it, the pack adds additional checks while picking the memory state
of the first load. When the store locates in a pack and the load pack
relies on the last_mem, we shouldn't choose the memory state of the
first load but choose the memory state of the last load.
[1]https://github.com/openjdk/jdk/blob/0ae834105740f7cf73fe96be22e0f564ad29b18d/src/hotspot/share/opto/superword.cpp#L2380
[2]https://github.com/openjdk/jdk/blob/0ae834105740f7cf73fe96be22e0f564ad29b18d/src/hotspot/share/opto/superword.cpp#L2232
Jira: ENTLLT-5482
Change-Id: I341d10b91957b60a1b4aff8116723e54083a5fb8
CustomizedGitHooks: yes
* Replace auto.sh to auto.rb also remove executable property from those launch scripts. * JVM-1878: [PEA] refactor auto.sh to support problem-list. * Add BadGraphVolatile and put it in the problem list. --------- Co-authored-by: Xin Liu <xxinliu@amazon.com>
* Also push upstream to master That will allow to run our GHA CI on vanilla upstream * Rework cron schedule to take GHA duration into account
…ng into ldp/stp on AArch64 Macro-assembler on aarch64 can merge adjacent loads or stores into ldp/stp[1]. For example, it can merge: ``` str w20, [sp, openjdk#16] str w10, [sp, openjdk#20] ``` into ``` stp w20, w10, [sp, openjdk#16] ``` But C2 may generate a sequence like: ``` str x21, [sp, openjdk#8] str w20, [sp, openjdk#16] str x19, [sp, openjdk#24] <--- str w10, [sp, openjdk#20] <--- Before sorting str x11, [sp, openjdk#40] str w13, [sp, openjdk#48] str x16, [sp, openjdk#56] ``` We can't do any merging for non-adjacent loads or stores. The patch is to sort the spilling or unspilling sequence in the order of offset during instruction scheduling and bundling phase. After that, we can get a new sequence: ``` str x21, [sp, openjdk#8] str w20, [sp, openjdk#16] str w10, [sp, openjdk#20] <--- str x19, [sp, openjdk#24] <--- After sorting str x11, [sp, openjdk#40] str w13, [sp, openjdk#48] str x16, [sp, openjdk#56] ``` Then macro-assembler can do ld/st merging: ``` str x21, [sp, openjdk#8] stp w20, w10, [sp, openjdk#16] <--- Merged str x19, [sp, openjdk#24] str x11, [sp, openjdk#40] str w13, [sp, openjdk#48] str x16, [sp, openjdk#56] ``` To justify the patch, we run `HelloWorld.java` ``` public class HelloWorld { public static void main(String [] args) { System.out.println("Hello World!"); } } ``` with `java -Xcomp -XX:-TieredCompilation HelloWorld`. Before the patch, macro-assembler can do ld/st merging for 3688 times. After the patch, the number of ld/st merging increases to 3871 times, by ~5 %. Tested tier1~3 on x86 and AArch64. [1] https://github.com/openjdk/jdk/blob/a95062b39a431b4937ab6e9e73de4d2b8ea1ac49/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp#L2079
…and match candidate yet. (openjdk#20) * First round of instance patterns. No patterns with separate instance and match candidate yet. * A very (VERY!) basic support for instance pattern with split receiver and match candidate. * Align calling convention between instance patterns and deconstructors * Fix adjustment of `syntheticPattern` computation --------- Co-authored-by: Angelos Bimpoudis <angelos.bimpoudis@oracle.com>
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/20/head:pull/20$ git checkout pull/20