jdk icon indicating copy to clipboard operation
jdk copied to clipboard

Published 20 hours ago • verified publisher icon openjdk

8332163: C2 SuperWord: refactor PacksetGraph and SuperWord::output into VTransformGraph

Open eme64 opened this issue 1 year ago • 6 comments

The original PR was here, it got too chaotic.

I added some extra tests for this in: https://github.com/openjdk/jdk/pull/19558 I extracted some refactorings to: https://github.com/openjdk/jdk/pull/19573

We used to have:

  • PacksetGraph: this detects cycles introduces by packs, and schedules/reorders the memops.
  • SuperWord::apply_vectorization: creates VectorNodes directly from the PackSet.

In my blog, I have published lots of ideas for SuperWord / AutoVectorization improvements: https://eme64.github.io/blog/2023/11/03/C2-AutoVectorizer-Improvement-Ideas.html

Many ideas are based on the "VectorTransform IR": cost-model, if-conversion, direct widening of scalars to vectors, additional optimizations/features with shuffle/pack/extract, handling more reduction patterns, etc.

I now decided to name it VTransform, which is essencially a graph VtransformGraph of nodes VTransformNodes that resemble the C2 Node on purpose, because the VTransform models the C2 graph after vectorization. We can now model the transformation from scalar-loop to vectorized-loop without modifying the C2 graph yet.

The new code has these steps:

  • Given the PackSet from SuperWord, we create a VTransformGraph with SuperWordVTransformBuilder.
  • [Not yet: all sorts of optimizations / checks on the VTransformGraph, in future RFE's]
  • We then schedule the VTransformGraph, and check for cycles.
  • Once we are ready to commit to vectorization, we call VTransformGraph::apply_vectorization which lets each individual VTransformNode::apply generate the new vectorized C2 nodes.

Testing

Regression testing passed.

Performance testing: no significant change in performance (as expected).

Progress

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

Issue

  • JDK-8332163: C2 SuperWord: refactor PacksetGraph and SuperWord::output into VTransformGraph (Enhancement - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19719

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

Using diff file

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

Webrev

Link to Webrev Comment

eme64 avatar Jun 14 '24 10:06 eme64

:wave: 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.

bridgekeeper[bot] avatar Jun 14 '24 10:06 bridgekeeper[bot]

@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:

8332163: C2 SuperWord: refactor PacksetGraph and SuperWord::output into VTransformGraph

Reviewed-by: chagedorn, kvn

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 2 new commits pushed to the master branch:

  • ced99066354fc6a32c587b9e3c35b07e26d3452e: 8334371: [AIX] Beginning with AIX 7.3 TL1 mmap() supports 64K memory pages
  • 916db07e533cdc0fca2010751f7ebe54e6ada7b9: 8335532: [JVMCI] Export VM_Version::L1_line_size in JVMCI

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch. 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[bot] avatar Jun 14 '24 10:06 openjdk[bot]

@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[bot] avatar Jun 14 '24 10:06 openjdk[bot]

Webrevs

mlbridge[bot] avatar Jun 16 '24 18:06 mlbridge[bot]

@chhagedorn thank you for the many suggestions, I will look into them now :)

eme64 avatar Jul 02 '24 12:07 eme64

@chhagedorn thanks again for all the review comments! I now addressed everything, except for the complaint that I repeatedly used _vtransform.arena(). But I don't know any better alternative. Your suggestion of using a _arena field in the SuperWordVTransformBuilder is not a great idea, because it suggests that this _arena has the life-time of the builder. But it should be the life-time of the vtransform, which outlives the builder. So it would have to be a _vtransform_arena field, at which point I might as well use _vtransform.arena() as it is now. Let me know if you have any better idea.

And please re-review ;)

eme64 avatar Jul 03 '24 14:07 eme64

@chhagedorn thanks for another round of reviews. I think I addressed all your comments :)

eme64 avatar Jul 04 '24 10:07 eme64

@chhagedorn thanks for the review, I added your suggestions!

eme64 avatar Jul 04 '24 11:07 eme64

Thanks @chhagedorn for the very in-depth and very helpful reviews and suggestions! Thanks @vnkozlov for reviewing too! /integrate

eme64 avatar Jul 08 '24 06:07 eme64

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

  • 3f37c5718d676b7001e6a084aed3ba645745a144: 8335806: RISC-V: Corrected typos Bizarrely
  • 6f7f0f1de05fdc0f6a88ccd90b806e8a5c5074ef: 8333884: MemorySegment::reinterpret removes read-only property
  • b83766e59063a41ea8801ac9e7c15dce67727c62: 8335632: jdk/jfr/api/consumer/streaming/TestJVMExit.java failed with "Process [...] is no longer alive"
  • 7efe16038e5df9894a265ea1214068060f595c4e: 8335730: JFR: Clean up jdk.jfr
  • ff49f677ee5017019c90823bc412ceb90068ffbd: 8335775: Remove extraneous 's' in comment of rawmonitor.cpp test file
  • 194425d7875ef42fce52516ed59c81ee97720399: 8335645: j.u.Formatter#trailingZeros improved with String repeat
  • c8acea87e2c5ba6672c011ec4e57a53c55fee74b: 8335706: G1: Remove unused G1ConcurrentRefine::RemSetSamplingClosure::_cset
  • bdf470b3b8f8814cb29f2877490d5bc1e79bdecb: 8335742: Problemlist gc/g1/TestMixedGCLiveThreshold.java#25percent with virtual threads
  • 6409ec336af647044d0746c219496ad070de5e9d: 8335711: G1: Remove unused bot_updates argument in G1AllocRegion constructor
  • 4ec1ae109710aa150e27acf5706475d335c4655c: 8331385: G1: Prefix HeapRegion helper classes with G1
  • ... and 10 more: https://git.openjdk.org/jdk/compare/c0604fb823d9f3b2e347a9857b11606b223ad8ec...master

Your commit was automatically rebased without conflicts.

openjdk[bot] avatar Jul 08 '24 06:07 openjdk[bot]

@eme64 Pushed as commit 02956ab6e161ca8556a73f328f79bcbfba997cbc.

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

openjdk[bot] avatar Jul 08 '24 06:07 openjdk[bot]