8325155: C2 SuperWord: remove alignment boundaries by eme64 · Pull Request #18822 · openjdk/jdk

eme64 · 2024-04-17T17:58:53Z

I have tried for a very long time to get rid of all the alignment(n) code that is all over the SuperWord code. With lots of previous work, I am now finally ready to remove it.

I was able to remove lots of VM code, about 300 lines. And the removed code is I think much more complicated than the new code.

This is what I did in this PR:

Removal of _node_info: used to have many fields, which I refactored out to the VLoopAnalyzer modules. alignment is the last component, which I now remove.
Changed the implementation of SuperWord::find_adjacent_refs, now SuperWord::find_adjacent_memop_pairs, completely:
- It used to be an algorithm that would scan over all memops repeatedly, try to find some mem_ref and see which other memops were comparable, and then pack pairs for all of those, by comparing all-vs-all memops. This algorithm is at least quadratic, if not much worse.
- I now add all memops into a single array, sort them by groups (those that are comparable with each other and could be packed into vectors), and inside the groups by ascending offset. This allows me to split off the groups much more efficiently, and also the sorting by offset allows me finding adjacent pairs much more efficiently. In the most cases this reduces the cost to O(n log n) for sort, and a linear scan for finding adjacent memops.
I removed the "alignment boundaries" created in SuperWord::memory_alignment by int off_rem = offset % vw;.
- This used to have the effect that all offsets were computed modulo the vector width. Hence, pairs could not be packed across this boundary (e.g. we have nodes with offsets 31, 32, which are adjacent in theory, but if we have a vw = 32, then the modulo-offsets are 31, 0, and they are not detected as adjacent).
- These "alignment boundaries" used to be required for correctness about a year ago, before I fixed and relaxed much of the alignment code.
The alignment used to have another important task: Ensuring compatibility of the input-size of a use node, with the output-size of the def-node.
- This was done by giving all nodes an alignment, even the non-memop nodes. This alignment was then scaled up and down at type casts (e.g. int 0, 4, 8, 12 -> long 0, 8, 16, 24). If the output-size of the def-node did not match the input-size of the use-node, then the alignment would not match up, and we would not pack.
- This is why we used to have checks like alignment(s1) + data_size(s1) == alignment(s2) and s2_align == align + data_size(s1), and why we did set_alignment(s2, align + data_size(s1)); inside SuperWord::set_alignment(Node* s1, Node* s2, int align).
- I decided to NOT check if use/def type sizes match during packing, but only much later in SuperWord::profitable (bad name, it has always been more about checking consistency than profitability, but I will rename that in a Future RFE). The relevant code is in SuperWord::is_velt_basic_type_compatible_use_def.

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-8325155: C2 SuperWord: remove alignment boundaries (Enhancement - P4)

Reviewers

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

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18822

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-17T17:59:35Z

👋 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 · 2024-04-17T17:59:48Z

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

8325155: C2 SuperWord: remove alignment boundaries

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

2a37764: 8333743: Change .jcheck/conf branches property to match valid branches
75dc2f8: 8330182: Start of release updates for JDK 24
054362a: 8332550: [macos] Voice Over: java.awt.IllegalComponentStateException: component must be showing on the screen to determine its location
9b436d0: 8333674: Disable CollectorPolicy.young_min_ergo_vm for PPC64
487c477: 8333647: C2 SuperWord: some additional PopulateIndex tests
d02cb74: 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3
02f2404: 8333560: -Xlint:restricted does not work with --release
606df44: 8332670: C1 clone intrinsic needs memory barriers
33fd6ae: 8333622: ubsan: relocInfo_x86.cpp:101:56: runtime error: pointer index expression with base (-1) overflowed
8de5d20: 8332865: ubsan: os::attempt_reserve_memory_between reports overflow
... and 155 more: https://git.openjdk.org/jdk/compare/da6aa2a86c86ba5fce747b36dcb2d6001cfcc44e...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 · 2024-04-17T18:00:28Z

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

eme64 · 2024-04-30T12:43:26Z

src/hotspot/share/opto/superword.cpp

+      }
+#endif
+    return true;
+  }


Note: compatibility with def used to be checked via alignment, but now we need to check via is_velt_basic_type_compatible_use_def. For reductions, we only check the "second" input.

eme64 · 2024-05-06T11:32:00Z

src/hotspot/share/opto/superword.cpp

  _vloop_analyzer(vloop_analyzer),
  _vloop(vloop_analyzer.vloop()),
  _arena(mtCompiler),
-  _node_info(arena(), _vloop.estimated_body_length(), 0, SWNodeInfo::initial), // info needed per node


Note: held the "alignment" info, all other fields were already removed in previous refactorings.

eme64 · 2024-05-06T11:32:12Z

src/hotspot/share/opto/superword.cpp

  _arena(mtCompiler),
-  _node_info(arena(), _vloop.estimated_body_length(), 0, SWNodeInfo::initial), // info needed per node
  _clone_map(phase()->C->clone_map()),                      // map of nodes created in cloning
-  _align_to_ref(nullptr),                                   // memory reference to align vectors to


Note: renamed it to _mem_ref_for_main_loop_alignment

eme64 · 2024-05-06T11:42:02Z

src/hotspot/share/opto/superword.cpp

-  }
-  if (longer_type_for_conversion(s) != T_ILLEGAL ||
-      longer_type_for_conversion(t) != T_ILLEGAL) {
-    align = align / data_size(s) * data_size(t);


Note: this check was there to ensure the type size of use/def nodes matches. This is now done by is_velt_basic_type_compatible_use_def.

eme64 · 2024-05-06T11:43:29Z

src/hotspot/share/opto/superword.cpp

-              return true;
-            }
-          }
+          return true;


Note: we still check are_adjacent_refs, and non-memops don't need any alignment.

mlbridge · 2024-05-13T08:03:02Z

Webrevs

chhagedorn

Great cleanup! I have some comments but otherwise, looks good.

chhagedorn · 2024-05-27T12:42:46Z

src/hotspot/share/opto/superword.cpp

+  GrowableArray<const VPointer*> vpointers;
+
+  // Collect all valid VPointers.
+  for_each_mem([&] (const MemNode* mem, int bb_idx) {


The different parts of this method could be nicely put into separate methods which reduces the size of find_adjacent_memop_pairs().

GrowableArray<const VPointer*> vpointers; collect_valid_vpointers(vpointers); vpointers.sort(); // trace code find_adjacent_memops(vpointers); // trace code

The entire "find adjacent memop pairs" code could also be put into a separate class but I leave it up to you to decide if it's worth or not.

A class would be nice, but I think I would have to pass around too much for that.
find_adjacent_memop_pairs_in_one_group requires some things like:

_do_vector_loop same_origin_idx can_pack_into_pair // especially this one

Not sure it is worth creating a separate class, I think it would become more complicated that way.

chhagedorn · 2024-05-27T13:12:58Z

src/hotspot/share/opto/superword.cpp

+  });
+
+  // Sort the VPointers. This does 2 things:
+  //  - Separate the VPointer into groups (e.g. all LoadI of the same base and invar). We only need to find adjacent memops inside


Maybe you should state here what a "group" is. It is only explained at cmp_for_sort_by_group().

src/hotspot/share/opto/superword.cpp

chhagedorn · 2024-05-27T13:22:23Z

src/hotspot/share/opto/superword.cpp

+    while (group_end < vpointers.length() &&
+           VPointer::cmp_for_sort_by_group(
+             vpointers.adr_at(group_start),
+             vpointers.adr_at(group_end)
+           ) == 0) {
+      group_end++;


This is somewhat hard to read. How about putting this into a separate method? I.e.

int group_start = 0; while (group_start < vpointers.length()) { int group_end = find_group_end(vpointers, group_start); // <---- EXTRACTED to new method find_adjacent_memop_pairs_in_group(vpointers, group_start, group_end); group_start = group_end; }

chhagedorn · 2024-05-27T13:24:27Z

src/hotspot/share/opto/superword.cpp

-    }
-  }
-
+// Find adjacent memops for a single group, e.g. for all LoadI of the same base, invar, etc.


You should mention here that this method finally adds a new pair to the _pairset. On a separate note, find_adjacent_memop_pairs_in_group() suggests that we find something but we actually "find and add" something without returning anything from the method. Should we make this more clear in the method name?

src/hotspot/share/opto/superword.cpp

chhagedorn · 2024-05-27T14:26:44Z

src/hotspot/share/opto/superword.cpp

  return true;
 }

+bool SuperWord::is_velt_basic_type_compatible_use_def(Node* use, int idx) const {


Maybe add a comment here to quickly explain that compatible means "output size of the def node matches the input size of the use node".

src/hotspot/share/opto/superword.cpp

chhagedorn · 2024-05-27T14:31:48Z

src/hotspot/share/opto/superword.cpp

    return true;
  }

+  if (!is_velt_basic_type_compatible_use_def(use, u_idx)) {


Might be easier to directly put in def as defined on L2764 instead of passing the index to the def.

Good point. I think I used to require idx in a previous iteration, but not any more!

src/hotspot/share/opto/superword.hpp

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

openjdk · 2024-05-28T17:31:03Z

⚠️ @eme64 This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

eme64 · 2024-05-28T17:32:19Z

Thanks @chhagedorn for the review! I think I addressed all your points. Except for this:

You should mention here that this method finally adds a new pair to the _pairset. On a separate note, find_adjacent_memop_pairs_in_group() suggests that we find something but we actually "find and add" something without returning anything from the method. Should we make this more clear in the method name?

I have not yet found a better name. I think find is ok here. I guess we could have it be find_and_add but this seems unnecessarily long to me.

chhagedorn

Thanks for doing the updates, looks good now! find -> create is a good solution :-)

src/hotspot/share/opto/superword.cpp

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

eme64 · 2024-05-30T06:30:32Z

FYI: I ran performance benchmarking, and there was no significant difference.

vnkozlov

Approved.

vnkozlov · 2024-06-04T15:51:23Z

Thank you for running performance testing.

eme64 · 2024-06-04T15:55:31Z

@vnkozlov thanks for the review! I will integrate as soon as the JDK24 fork happens ;)

eme64 · 2024-06-07T05:00:04Z

Thanks @chhagedorn @vnkozlov for the reviews!
/integrate

openjdk · 2024-06-07T05:01:25Z

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

d8af589: 8026127: Deflater/Inflater documentation incomplete/misleading
6238bc8: 8333456: CompactNumberFormat integer parsing fails when string has no suffix
2a37764: 8333743: Change .jcheck/conf branches property to match valid branches
75dc2f8: 8330182: Start of release updates for JDK 24
054362a: 8332550: [macos] Voice Over: java.awt.IllegalComponentStateException: component must be showing on the screen to determine its location
9b436d0: 8333674: Disable CollectorPolicy.young_min_ergo_vm for PPC64
487c477: 8333647: C2 SuperWord: some additional PopulateIndex tests
d02cb74: 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3
02f2404: 8333560: -Xlint:restricted does not work with --release
606df44: 8332670: C1 clone intrinsic needs memory barriers
... and 157 more: https://git.openjdk.org/jdk/compare/da6aa2a86c86ba5fce747b36dcb2d6001cfcc44e...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-06-07T05:01:31Z

@eme64 Pushed as commit 944aeb8.

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

8325155

52814c0

openjdk bot changed the title ~~8325155~~ 8325155: C2 SuperWord: remove alignment boundaries Apr 17, 2024

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Apr 17, 2024

eme64 added 18 commits April 18, 2024 15:46

implement find_adjacent_memop_pairs_in_group

0f796fa

cleanup

ff183e1

rm old code

914f8f7

remove more alignment code

59475db

rename align_to_ref -> _mem_ref_for_main_loop_alignment

fd4a290

AutoVectorization flags adjusted

09e273a

implement is_velt_basic_type_compatible_use_def

1d78a46

small bugfix for memory nodes

1d38cd1

stub for test TestCompatibleUseDefTypeSize.java

0e6e3b5

deflate test

8f81625

test where removal of alignment boundaries makes a difference

60e89c0

some F/I and D/L tests

e917f20

update IR rules for additional vectorization in TestSplitPacks.java

321fdd3

Merge branch 'master' into JDK-8325155-rm-alignment-boundaries

a2e7387

Merge branch 'master' into JDK-8325155-rm-alignment-boundaries

e711c0e

Merge branch 'master' into JDK-8325155-rm-alignment-boundaries

1b15867

MaxI reduction over chars

b0221f2

fix reductions

6b8b69d

eme64 commented Apr 30, 2024

View reviewed changes

fix alignment

b57a35b

eme64 commented May 6, 2024

View reviewed changes

eme64 marked this pull request as ready for review May 13, 2024 07:59

openjdk bot added the rfr Pull request is ready for review label May 13, 2024

Merge branch 'master' into JDK-8325155-rm-alignment-boundaries

82c9a77

chhagedorn reviewed May 28, 2024

View reviewed changes

eme64 and others added 3 commits May 28, 2024 18:47

Apply suggestions from code review by Christian

d48bafa

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

more updates for Christian

6aee526

merge

54bfbb0

eme64 added 2 commits May 28, 2024 19:33

Merge branch 'master' into JDK-8325155-rm-alignment-boundaries

41fc1eb

find_adjacent_memop_pairs -> create_adjacent_memop_pairs

cd65ca0

chhagedorn approved these changes May 29, 2024

View reviewed changes

src/hotspot/share/opto/superword.cpp Outdated Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label May 29, 2024

Update src/hotspot/share/opto/superword.cpp

d18219b

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

chhagedorn approved these changes May 29, 2024

View reviewed changes

vnkozlov approved these changes Jun 4, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 7, 2024

openjdk bot closed this Jun 7, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 7, 2024

This was referenced Jun 16, 2024

8333876: C2 SuperWord: regression after JDK-8325155: failed: internal connection #19736

Closed

8334083: C2 SuperWord: TestCompatibleUseDefTypeSize.java fails with -XX:+AlignVector after JDK-8325155 #19801

Closed

eme64 mentioned this pull request May 15, 2025

8355094: Performance drop in auto-vectorized kernel due to split store #25065

Closed

3 tasks

Conversation

eme64 commented Apr 17, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Apr 17, 2024

Uh oh!

openjdk bot commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Apr 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented May 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

chhagedorn left a comment

Choose a reason for hiding this comment

Uh oh!

chhagedorn May 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openjdk bot commented May 28, 2024

Uh oh!

eme64 commented May 28, 2024

Uh oh!

chhagedorn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eme64 commented May 30, 2024

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

vnkozlov commented Jun 4, 2024

Uh oh!

eme64 commented Jun 4, 2024

Uh oh!

eme64 commented Jun 7, 2024

Uh oh!

openjdk bot commented Jun 7, 2024

Uh oh!

openjdk bot commented Jun 7, 2024

eme64 commented Apr 17, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 17, 2024 •

edited

Loading

mlbridge bot commented May 13, 2024 •

edited

Loading

chhagedorn May 27, 2024 •

edited

Loading

chhagedorn left a comment •

edited

Loading