8335392: C2 MergeStores: enhanced pointer parsing

[eme64]

Background
I am introducing the MemPointer, for enhanced pointer parsing. For now, it replaces the much more limited ArrayPointer in MergeStores (see #16245), but eventually it is supposed to be used widely in optimizations for pointer analysis: adjacency, aliasing, etc. I also plan to refactor the VPointer from auto-vectorization with it, and unlock more pointer patterns that way - possibly including scatter/gather.

Details

The MemPointer decomposes a pointer into the form pointer = con + sum_i(scale_i * variable_i) - a linear form with a sum of variables and scale-coefficients, plus some constant offset.

This form allows us to perform aliasing checks - basically we can check if two pointers are always at a constant offset. This allows us to answer many questions, including if two pointers are adjacent. MergeStores needs to know if two stores are adjacent, so that we can safely merge them.

More details can be found in the description in mempointer.hpp. Please read them when reviewing!

MemPointer is more powerful than the previous ArrayPointer: the latter only allows arrays, the former also allows native memory accesses, Unsafe and MemorySegement.

What this change enables

Before this change, we only allowed merging stores to arrays, where the store had to have the same type as the array element (StoreB on byte[], StoreI on int[]).

Now we can do:

• Merging Unsafe stores to array. Including "mismatched size": e.g. putChar to byte[].
• Merging Unsafe stores to native memory.
• Merging MemorySegment: with array, native, ByteBuffer backing types.
  • However: there is still some problem with RangeCheck smearing (a type of RC elimination) for the examples I have tried. Without RC's smeared, we can only ever merge 2 neighbouring stores. I hope we can improve this with better RangeCheck smearing. MemorySegment introduce checkIndexL, the long-variant of the RangeCheck. Normal array accesses only use the equivalent of checkIndex, the int-variant that we already optimize away much better.

Dealing with Overflows

We have to be very careful with overflows when dealing with pointers. For this, I introduced a NoOverflowInt. It allows us to do "normal" int operations on it, and tracks if there was ever an overflow. This way, we can do all overflow checks implicitly, and do not clutter the code with overflow-checks or - God forbid - forget overflow-checks.

Benchmarks

I added a few new benchmarks, to show the merging of Unsafe and native stores. We an see that 8 byte stores are now merged, and have the same performance as a long store. The same for 4 char stores that are merged into a single long store.

And here the whole MergeStores benchmark, master and with patch:

All old benchmarks remain in the level of noise. A few have heavy errors - the benchmark ran on my laptop and maybe something slowed down for a bit. We can see the significant speedup with all the unsafe benchmarks at the bottom.

This is great, now we can perform putChar even on byte[]. This was an issue we discovered in this PR #19626.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8335392: C2 MergeStores: enhanced pointer parsing (Enhancement - P4)

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer)
• Christian Hagedorn (@chhagedorn - Reviewer)

Contributors

• Christian Hagedorn <[email protected]>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19970/head:pull/19970
$ git checkout pull/19970

Update a local copy of the PR:
$ git checkout pull/19970
$ git pull https://git.openjdk.org/jdk.git pull/19970/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19970

View PR using the GUI difftool:
$ git pr show -t 19970

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19970.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back epeter! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@eme64 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8335392: C2 MergeStores: enhanced pointer parsing

Co-authored-by: Christian Hagedorn <[email protected]>
Reviewed-by: kvn, chagedorn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 32 new commits pushed to the master branch:

• 7f8450c: 8343473: Update copyright year of AddmodsOption.java
• b74652b: 8343167: Unnecessary define checks in InterpreterRuntime after JDK-8199809
• 646d64e: 8340307: Add explanation around MemorySegment:reinterpret regarding arenas
• 8d6cfba: 8336267: Method and Constructor signature parsing can be shared on the root object
• 1f7d524: 8343437: ClassDesc.of incorrectly permitting empty names
• 895a7b6: 8342967: Lambda deduplication fails with non-metafactory BSMs and mismatched local variables names
• b41d713: 8343513: Forward declare Thread in mutexLocker.hpp
• 809030b: 8321500: javadoc rejects '@' in multi-line attribute value
• 7bca0af: 8343128: PassFailJFrame.java test result: Error. Bad action for script: build}
• f69b601: 8343188: Investigate ways to simplify MemorySegment::ofBuffer
• ... and 22 more: https://git.openjdk.org/jdk/compare/f77a5144a12fc31bad8b672a3cc9caa688d78e72...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@eme64 The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[openjdk]

@eme64 this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8335392-MemPointer
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[chhagedorn]

Some final last mostly minor comments but otherwise, it looks good to me now! I like the summaries and how you worked out the proofs. They are now easy to understand.

[eme64]

/contributor add chhagedorn

You spent enough time on this already ;)

[openjdk]

@eme64 chhagedorn was not found in the census.

Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

• /contributor add @openjdk-bot
• /contributor add duke
• /contributor add J. Duke <[email protected]>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

[eme64]

/contributor add @chhagedorn

[openjdk]

@eme64
Contributor Christian Hagedorn <[email protected]> successfully added.

[chhagedorn]

/contributor add chhagedorn

You spent enough time on this already ;)

Thanks Emanuel, I highly appreciate that :-)

[eme64]

@chhagedorn I filed https://bugs.openjdk.org/browse/JDK-8343536 to track the cases in TestMergeStoresMemorySegment.java that do not optimize as hoped for.

[eme64]

@chhagedorn I addressed all your review suggestions. Thank you very much for the in-depth review :)

[chhagedorn]

That looks good, thanks for the patience to work through all the suggestions and also for the offline discussions!

[eme64]

@vnkozlov Would you like to re-review? If I don't hear anything then I'll integrate tomorrow.

[vnkozlov]

Update is good.

[eme64]

Thanks @chhagedorn for the extensive reviews and collaboration on improving the proofs 😊
Thanks @vnkozlov for the approval.

I did an offline merge and testing (to avoid requiring a re-approval) - all looks good.

/integrate

[openjdk]

Going to push as commit f3671be.
Since your change was applied there have been 47 commits pushed to the master branch:

• 4fc6d41: 8341194: [REDO] Implement C2 VectorizedHashCode on AArch64
• abf2dc7: 8343298: Improve stability of runtime/cds/DeterministicDump.java test
• dafa2e5: 8343124: Tests fails with java.lang.IllegalAccessException: class com.sun.javatest.regtest.agent.MainWrapper$MainTask cannot access
• 0f7dd98: 8251926: PPC: Remove an unused variable in assembler_ppc.cpp
• cd91a44: 8343549: SeededSecureRandomTest needn't be in a package
• 20f3aaf: 8343471: RISC-V: compiler/cpuflags/TestAESIntrinsicsOnUnsupportedConfig.java fails after JDK-8334999
• 67907d5: 8343500: Optimize ArrayClassDescImpl computeDescriptor
• 714472d: 8341798: Fix ExceptionOccurred in jdk.jdwp.agent
• 825ceb1: 8341796: Fix ExceptionOccurred in jdk.hotspot.agent
• 8b47497: 8331682: Slow networks/Impatient clients can potentially send unencrypted TLSv1.3 alerts that won't parse on the server
• ... and 37 more: https://git.openjdk.org/jdk/compare/f77a5144a12fc31bad8b672a3cc9caa688d78e72...master

Your commit was automatically rebased without conflicts.

[openjdk]

@eme64 Pushed as commit f3671be.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[wenshao]

@eme64 How do I use the TraceMergeStores option? It worked before, but now it gives an error.

build/macosx-aarch64-server-fastdebug/jdk/bin/java -Dtest=appendNullLatin1 -XX:+TraceMergeStores

output

Unrecognized VM option 'TraceMergeStores'
Did you mean '(+/-)MergeStores'?
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

[eme64]

@wenshao Ah, good question! I changed it from a "global" flag to a "compile option". You can now filter the methods! And you can enable different tags - so you can regulate how verbose it is. Example:
-XX:CompileCommand=TraceMergeStores,Test::test*,SUCCESS,ADJACENCY,ALIASING,BASIC

And to see all available tags:
-XX:CompileCommand=TraceMergeStores,Test::test*,help

Usage for CompileCommand TraceMergeStores:
  -XX:CompileCommand=TraceMergeStores,<package.class::method>,<tags>
  tags                   descriptions
  BASIC                  Trace basic analysis steps
  POINTER                Trace pointer IR
  ALIASING               Trace MemPointerSimpleForm::get_aliasing_with
  ADJACENCY              Trace adjacency
  SUCCESS                Trace successful merges

You might have to play around a little to see what is helpful to you. And I'm always open to feedback :)

[wenshao]

Currently, TraceMergeStores can only be used in fastdebug images. Are you planning to support it in release images?

[eme64]

Currently, TraceMergeStores can only be used in fastdebug images. Are you planning to support it in release images?

I generally don't support it in release builds, only debug - or rather NOT_PRODUCT. The issue with supporting in release is that some other printing methods I use are not available in product (Node::dump). And if we support it in release, then we have to create a CSR, clearly specify what it prints, and then we are going to be less flexible in the future with changing the behavior. So I would rather not make it product, at least for now ;)
BTW: this is also why you can only disable -XX:-MergeStores with -XX:+UnlockDiagnosticVMOptions : it is not a full product flag, and so does not require a CSR, and we are able to remove it or change its behavior. But if someone really has an issue with the MergeStores optimization, they at least have a workaround until we are able to fix it ;)

Does that make sense?

[wenshao]

If it is not provided in the release image, users need to find the source code of the current version of JDK to build the fastdebug image to analyze whether the MergeStore optimization of a certain code works.

I can understand that MergeStore may still need to be improved, so it cannot be used as a product feature, but this is a useful optimization and I hope it can be provided in the product eventually.

I hope that TraceMergeStore can eventually be used in the release image like PrintInlining and become a tool for performance optimizers.

[eme64]

@wenshao I suppose we could consider making TraceMergeStores and TraceAutoVectorization available in product, but under the -XX:+UnlockDiagnosticVMOptions flag... I will discuss this with other VM engineers. That means it is available, but there is no promise of stability. Still, once people become dependent on it, maybe even tools become dependent, then it is harder to make changes without everybody complaining 😅