jdk icon indicating copy to clipboard operation
jdk copied to clipboard

Published 20 hours ago • verified publisher icon openjdk

8325252: C2 SuperWord: refactor the packset

Open eme64 opened this issue 11 months ago • 3 comments

I'm refactoring the packset, separating the details of packset-manupulation from the SuperWord algorithm.

Most importantly: I split it into two classes: PairSet and PackSet. combine_pairs_to_longer_packs converts the first into the second. I was able to simplify the combining, and remove the pack-sorting. I now walk "pair-chains" directly with PairSetIterator.

More details are described in the annotations in the code.

TODO compare compile time!

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-8325252: C2 SuperWord: refactor the packset (Sub-task - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18276

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

Using diff file

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

eme64 avatar Mar 13 '24 14:03 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 Mar 13 '24 14:03 bridgekeeper[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 Mar 13 '24 14:03 openjdk[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:

8325252: C2 SuperWord: refactor the packset

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

  • 892b8bb6d1725119e4c9ada8f2617c15f8af674e: 8329189: runtime/stack/Stack016.java fails on libgraal
  • 05854fd704cba6ebd73007d9547a064891d49587: 8329163: C2: possible overflow in PhaseIdealLoop::extract_long_range_checks()
  • 614db2ea9e10346475eef34629eab54878aa482d: 8328638: Fallback option for POST-only OCSP requests
  • d292aabf05edf41e14eca1976142f63c7b54779e: 8329086: Clean up java.desktop native compilation
  • d0a265039a36292d87b249af0e8977982e5acc7b: 8324774: Add DejaVu web fonts
  • 37a5a08378fa686cda97357a0ad2632c359c98b9: 8329102: Clean up jdk.jpackage native compilation
  • 788d2bc40c0340ae5d157c84ce9c101a25464785: 8329169: Parallel: Remove unused local variable in MutableSpace::print_on
  • 4dfcc6df173d91b9b0b73d1ac3008e7cbb283871: 8329115: Crash involving return from inner switch
  • 9e566d76d1d8acca27d8f69fffcbeb0b49b060ba: 8327971: Multiple ASAN errors reported for metaspace
  • 8fc9097b3720314ef7efaf1f3ac31898c8d6ca19: 8315575: Retransform of record class with record component annotation fails with CFE
  • ... and 264 more: https://git.openjdk.org/jdk/compare/251347bd7e589b51354a2318bfac0c71cd71bf5f...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[bot] avatar Mar 13 '24 20:03 openjdk[bot]

Webrevs

mlbridge[bot] avatar Mar 20 '24 15:03 mlbridge[bot]

@chhagedorn I think I have addressed all your remarks. I agree I could have probably split the PairSet and PackSet into separate RFE's, that only became apparent once I did the work, and that there is not really any shared code between the two. Though I'm not sure it is worth splitting this up now, after all they are really about the _packset.

eme64 avatar Mar 25 '24 13:03 eme64

@chhagedorn thanks very much for the review, there were some great suggestions! I fixed almost all, and left some responses at the others.

I'll repeat them here:

You are right, I could actually remove the SuperWord reference from the PacksetGraph completely, and just pass the components, as const references. That would be really nice.

If it is ok for you, I will do this in a future RFE.

Actually, I plan to completely overhaul the PacksetGraph. It will be transformed to the VTransformGraph, and it will do:

Cycle checking (like today) Evaluate the cost-model Execute: each node knows how to replace its packed scalar nodes with vector nodes (basically refactoring SuperWord::output away) etc. We may even be able to take the VTransformGraph and try to widen all nodes, or make transformations on this graph, a simplified version of IGVN. I have lots of ideas that would be unlocked with this new graph-based approach.

We moved the implementation of split / filter to the packset, and that makes sense: we can hide the packset internals, and don't have them spill out to the SuperWord code. We can just call _packset.split_packs with a split_strategy. But defining the split strategy itself depends on other SuperWord components, and so I think conceptually they belong with SuperWord. They query reductions, dependency graph, implemented, AlignmentSolution, profitable, etc. Sure, we can just pass a SuperWord reference, but that does not really seem right to me either. For me, the SuperWord class is there to manage the interface between all the components, and try to avoid passing components to other components, wherever possible. So I would rather have a list of methods in SuperWord, and each such method defines how the components interact (e.g. packset and alignment, packset and AlignmentSolution, pairset and packset, ...). But we can discuss this further, and maybe come up with an even better solution. My hope is just that we separate the components as much as possible, so that we know that only a handful of them interact at a given point. That makes the whole beast more managable.

Using Pack instead of Node_List. Great idea, would improve readability. Let's do it in a future RFE.

eme64 avatar Mar 27 '24 15:03 eme64

Thanks @chhagedorn for the detailed review, thanks also @vnkozlov ! /integrate

eme64 avatar Apr 02 '24 06:04 eme64

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

  • 6b1b0e9d45eb56f88398e2a6bca0d90c03112eaa: 8329103: assert(!thread->in_asgct()) failed during multi-mode profiling
  • bc546c21a59d2481ba86f98d0d653c7691f68d4c: 8328561: test java/awt/Robot/ManualInstructions/ManualInstructions.java isn't used
  • af7c6af0cc1eb6c42199c05933c7feb032bd6353: 8324808: Manual printer tests have no Pass/Fail buttons, instructions close set 3
  • d3fc8df8af11d7cc1cc341bc75e46b7e93d6db31: 8329135: Store Universe::*exception_instance() in CDS archive
  • a85c8493aec73e81c000ea3e3d983b05706bbfec: 8328273: sun/management/jmxremote/bootstrap/RmiRegistrySslTest.java failed with java.rmi.server.ExportException: Port already in use
  • 70c8ff1c9a9adf21a16d8a6b4da1eeec65afe61d: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout
  • ecd2b7112a7617b11997da341d6185756bf1fb0f: 8329354: java/text/Format/MessageFormat/CompactSubFormats.java fails
  • c2979c150bdbcc2a9e6026347dc590e6a7e86595: 8329425: ProblemList containers/docker/TestJFREvents.java on linux-x64
  • 5698f7ad29c939b7e52882ace575dd7113bf41de: 8329134: Reconsider TLAB zapping
  • 4a14cba2f1632c5cb91e37a07638ea6d8ad4ec00: 8329213: Better validation for com.sun.security.ocsp.useget option
  • ... and 305 more: https://git.openjdk.org/jdk/compare/251347bd7e589b51354a2318bfac0c71cd71bf5f...master

Your commit was automatically rebased without conflicts.

openjdk[bot] avatar Apr 02 '24 06:04 openjdk[bot]

@eme64 Pushed as commit 5cddc2de493d9d8712e4bee3aed4f1a0d4c228c3.

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

openjdk[bot] avatar Apr 02 '24 06:04 openjdk[bot]