8319822: Use a linear-time algorithm for assert_different_registers() #16617

theRealAph · 2023-11-10T15:46:59Z

At the present time, assert_different_registers() uses an O(N**2) algorithm in assert_different_registers(). We can utilize RegSet to do it in O(N) time. This would be a useful optimization for all builds with assertions enabled.

In addition, it would be useful to be able to static_assert different registers.

Also, I've taken the opportunity to expand the maximum size of a RegSet to 64 on 64-bit platforms.

I also fixed a bug: sometimes noreg is passed to assert_different_registers(), but it may only be passed once or a spurious assertion is triggered.

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-8319822: Use a linear-time algorithm for assert_different_registers() (Enhancement - P4)

Reviewers

Kim Barrett (@kimbarrett - Reviewer)
Stefan Karlsson (@stefank - Reviewer) ⚠️ Review applies to 693df766
Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to eac03cc1

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16617

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-11-10T15:48:18Z

👋 Welcome back aph! 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 · 2023-11-10T15:49:19Z

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

mlbridge · 2023-11-10T15:52:08Z

Webrevs

TobiHartmann · 2023-11-13T08:06:43Z

The build fails on Windows:

[2023-11-13T08:04:09,465Z] c:\sb\prod\1699862498\workspace\open\src\hotspot\cpu\x86\register_x86.hpp(402): error C2220: the following warning is treated as an error
[2023-11-13T08:04:09,465Z] c:\sb\prod\1699862498\workspace\open\src\hotspot\cpu\x86\register_x86.hpp(402): warning C4267: 'argument': conversion from 'size_t' to 'int', possible loss of data
[2023-11-13T08:04:09,465Z] c:\sb\prod\1699862498\workspace\open\src\hotspot\cpu\x86\register_x86.hpp(415): warning C4267: 'argument': conversion from 'size_t' to 'int', possible loss of data
[2023-11-13T08:04:09,465Z] lib/CompileGtest.gmk:94: recipe for target '/cygdrive/c/sb/prod/1699862498/workspace/build/windows-x64-open/hotspot/variant-server/libjvm/gtest/objs/static/BUILD_GTEST_LIBJVM_pch.obj' failed
[2023-11-13T08:04:09,465Z] make[3]: *** [/cygdrive/c/sb/prod/1699862498/workspace/build/windows-x64-open/hotspot/variant-server/libjvm/gtest/objs/static/BUILD_GTEST_LIBJVM_pch.obj] Error 1

theRealAph · 2023-11-13T09:54:59Z

The build fails on Windows:

Argh! Thanks.

stefank · 2023-11-13T10:57:26Z

I started to review the patch and was wondering if this could be simplify to something like this?:
stefank@f38c791

I tested this with this small section of temporary static_asserts:
stefank@30da4d6

Unfortunately, that didn't compile and I had make this change to get it to work:
stefank@d6bda1a

theRealAph · 2023-11-15T16:23:08Z

I started to review the patch and was wondering if this could be simplify to something like this?: stefank@f38c791

Sure, it could be done. This is a minor efficiency tweak.

dean-long · 2023-11-15T22:53:40Z

I ran into a case where I was doing assert_different_registers() on base() and index() from an Address. How hard would it be to have assert_different_registers() support an Address as an argument?

shqking · 2023-11-16T02:52:55Z

The build fails on Windows:

Argh! Thanks.

I'm afraid the compiler warnings on Windows still exist. See the GHA test results.
Besides, the copyright year of register.hpp file should be updated to 2023.

theRealAph · 2023-11-17T15:28:37Z

I ran into a case where I was doing assert_different_registers() on base() and index() from an Address. How hard would it be to have assert_different_registers() support an Address as an argument?

Quite tricky, partly because Address isn't a shared type. I would have thought it best to make it explicit anyway, with something like
different_registers(a.base(), a.index(), rsi, ...);
We'd need to make sure that base() and index() return noreg whenever the Address is wrong, e.g. a literal.

stefank · 2023-11-20T10:28:49Z

I started to review the patch and was wondering if this could be simplify to something like this?: stefank@f38c791

Sure, it could be done. This is a minor efficiency tweak.

It tested the build performance before this PR, with the patch in this PR, and my simplified version. I can't see any performance difference on my MacBook M1. Is there any platform where this makes a bigger difference?

Edit: I realize that since this doesn't always boil down to a constexpr, then the run time might be more interesting than the build time.

stefank · 2023-11-20T10:32:12Z

From the summary:

In addition, it would be useful to be able to static_assert different registers.

As mentioned in #16617 (comment) this doesn't work unless we make the proposed small tweak. Do you want to make it in this PR, or should I propose that in a separate PR?

theRealAph · 2023-11-27T10:43:30Z

I started to review the patch and was wondering if this could be simplify to something like this?: stefank@f38c791

Sure, it could be done. This is a minor efficiency tweak.

It tested the build performance before this PR, with the patch in this PR, and my simplified version. I can't see any performance difference on my MacBook M1. Is there any platform where this makes a bigger difference?

Edit: I realize that since this doesn't always boil down to a constexpr, then the run time might be more interesting than the build time.

I don't think you'll be able to measure such a tiny difference in build performance, especially given all the other thiungs that are going on, and especially on a Great Big Out-Of-Order machine. But at runtime, the three-operand check can be performed with just three comparisons. I'm not fixated on this, though, and I can take the special cases out.

theRealAph · 2023-11-27T12:59:07Z

Unfortunately, that didn't compile and I had make this change to get it to work: stefank@d6bda1a

Done, thanks and sorry for the delay.

bridgekeeper · 2023-12-25T18:34:43Z

@theRealAph 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!

theRealAph · 2024-05-10T16:12:44Z

From the summary:

In addition, it would be useful to be able to static_assert different registers.

As mentioned in #16617 (comment) this doesn't work unless we make the proposed small tweak. Do you want to make it in this PR, or should I propose that in a separate PR?

Let's do it separately. I would, but GCC has a very relaxed attitude to static_assert which means I can't test anything here. Everything to do with static_assert just seems to work.

theRealAph · 2024-05-10T16:13:10Z

Exhuming this one after a long time. Please review, thanks.

stefank

Approved. I've written some suggestions that I would prefer, but that are not strictly necessary before integration.

src/hotspot/share/asm/register.hpp

theRealAph · 2024-05-28T15:53:15Z

I started to review the patch and was wondering if this could be simplify to something like this?: stefank@f38c791

Sure, it could be done. This is a minor efficiency tweak.

It tested the build performance before this PR, with the patch in this PR, and my simplified version. I can't see any performance difference on my MacBook M1. Is there any platform where this makes a bigger difference?

Edit: I realize that since this doesn't always boil down to a constexpr, then the run time might be more interesting than the build time.

You won't see very much, if any, because other things dominate. The main advantage, going forward, is that much of this can be constexpr'd, once I find out how to test on Windows.

Co-authored-by: Stefan Karlsson <stefan.karlsson@oracle.com>

kimbarrett · 2024-05-29T19:06:33Z

src/hotspot/cpu/x86/register_x86.hpp

-  uint32_t first = _bitset & -_bitset;
-  return first ? as_Register(exact_log2(first)) : noreg;
+  size_t first = _bitset & -_bitset;
+  return first != 0 ? as_Register(exact_log2(first)) : noreg;


This could instead be

if (_bitset == 0) { return noreg; } return as_register(count_trailing_zeros(_bitset));

which would be consistent with how last is being calculated. Note that exact_log2 bottoms
out in count_trailing_zeros.

Similarly for the XMMRegister case below.

kimbarrett · 2024-05-29T19:23:37Z

src/hotspot/share/asm/register.hpp

@@ -245,14 +253,44 @@ inline ReverseRegSetIterator<RegImpl> AbstractRegSet<RegImpl>::rbegin() {

 // Debugging support

+template<typename R, typename... Rx>
+inline constexpr bool different_registers(AbstractRegSet<R> allocated_regs, R first_register) {


Good point about static_assert. Don't make it debug-only.

kimbarrett · 2024-05-29T19:43:31Z

src/hotspot/share/asm/register.hpp

@@ -245,14 +253,35 @@ inline ReverseRegSetIterator<RegImpl> AbstractRegSet<RegImpl>::rbegin() {

 // Debugging support

+template<typename R, typename... Rx>


Rx is unused.

kimbarrett · 2024-05-29T19:48:33Z

src/hotspot/share/asm/register.hpp

@@ -245,14 +253,44 @@ inline ReverseRegSetIterator<RegImpl> AbstractRegSet<RegImpl>::rbegin() {

 // Debugging support

+template<typename R, typename... Rx>
+inline constexpr bool different_registers(AbstractRegSet<R> allocated_regs, R first_register) {


"inline" is still redundant with "constexpr".

theRealAph · 2024-05-31T16:59:02Z

Just a code-style review.

Question: could there be some sort of regression test for this, with different examples and edge cases?

I have no idea, really. assert_different_registers is used all over the place, and I'm going for bootcycle and tier1.

kimbarrett

Looks good.

src/hotspot/share/asm/register.hpp

tstuefe · 2024-06-01T06:00:55Z

Just a code-style review.
Question: could there be some sort of regression test for this, with different examples and edge cases?

I have no idea, really. assert_different_registers is used all over the place, and I'm going for bootcycle and tier1.

You could write a death test gtest. Like this:

TEST_VM_ASSERT_MSG(AssemblerAArch64, assert_different_regs, ".*Multiple uses of register: c_rarg0.*") {
  Register reg1 = r0;
  Register reg2 = r0;
  assert_different_registers(reg1, reg2);
}

theRealAph · 2024-06-03T08:30:56Z

Just a code-style review.
Question: could there be some sort of regression test for this, with different examples and edge cases?

I have no idea, really. assert_different_registers is used all over the place, and I'm going for bootcycle and tier1.

You could write a death test gtest. Like this:
TEST_VM_ASSERT_MSG(AssemblerAArch64, assert_different_regs, ".*Multiple uses of register: c_rarg0.*") {
  Register reg1 = r0;
  Register reg2 = r0;
  assert_different_registers(reg1, reg2);
}

I could, but I don't think there's much point. assert_different_registers() is used so much that it'll get thoroughly tested in the positive cases, at least. Do you think this is important?

tstuefe

I could, but I don't think there's much point. assert_different_registers() is used so much that it'll get thoroughly tested in the positive cases, at least. Do you think this is important?

See my remarks. I am mainly concerned about exceeding the range for the bit set. If you add the proposed static assert, I think we are good.

tstuefe · 2024-06-03T09:00:38Z

src/hotspot/share/asm/register.hpp

@@ -93,67 +93,75 @@ template <class RegImpl> class ReverseRegSetIterator;
 // A set of registers
 template <class RegImpl>
 class AbstractRegSet {
-  uint32_t _bitset;
+  size_t _bitset;


Why couple the number of possible registers to the memory size? Why not uint64_t?

64-bit makes sense.

I think this may have been broken for ppc where the number of vector registers can exceed 32 (

jdk/src/hotspot/cpu/ppc/register_ppc.hpp

Line 378 in 91101f0

number_of_registers = 64

)

Unless I am mistaken, assert_different_registers, if applied to VSR32 and up, would never have fired.

OK, I get it. I was sort-of thinking that on the 32-bit platforms we support we don't ever have more than 32 registers in a set, but maybe that's not true. I certainly don't want to slow down 32-bit platforms by burdening them with double-word operations for something that can never happen.

Okay. Well, if we have a static assert, we will notice if we have more registers than fit the bitset.

So I'm getting this bizarre failure on arm32. I'm guessing it's actually a compiler bug, but I suppose it might be some dusty corner of C++ to do with template arg substitution. Any thoughts?

In file included from /home/runner/work/jdk/jdk/src/hotspot/share/utilities/globalDefinitions.hpp:29, from /home/runner/work/jdk/jdk/src/hotspot/share/nmt/memflags.hpp:28, from /home/runner/work/jdk/jdk/src/hotspot/share/memory/allocation.hpp:29, from ad_arm.hpp:30, from ad_arm.cpp:28: /home/runner/work/jdk/jdk/src/hotspot/share/asm/register.hpp: In instantiation of ‘class AbstractRegSet<RegisterImpl*>’: /home/runner/work/jdk/jdk/src/hotspot/share/asm/register.hpp:272:30: required from ‘constexpr bool different_registers(R, Rx ...) [with R = RegisterImpl*; Rx = {RegisterImpl*, RegisterImpl*}]’ /home/runner/work/jdk/jdk/src/hotspot/share/asm/register.hpp:278:27: required from ‘void assert_different_registers(R, Rx ...) [with R = RegisterImpl*; Rx = {RegisterImpl*, RegisterImpl*}]’ /home/runner/work/jdk/jdk/src/hotspot/cpu/arm/arm.ad:8984:52: required from here /home/runner/work/jdk/jdk/src/hotspot/share/asm/register.hpp:96:26: error: ‘number_of_registers’ is not a member of ‘RegisterImpl*’ 96 | STATIC_ASSERT(RegImpl::number_of_registers <= 64); | ^~~~~~~~~~~~~~~~~~~ /home/runner/work/jdk/jdk/src/hotspot/share/utilities/debug.hpp:287:44: note: in definition of macro ‘STATIC_ASSERT’ 287 | #define STATIC_ASSERT(Cond) static_assert((Cond), #Cond) | ^~~~

BTW Would the runtime assert not prevent this from used as constexpr?

It doesn't, and I just saw the problem: on Arm, Register is a pointer type, whereas on other ports it's a class type.

Looks like our comments crossed over. I'm building Zero and it's OK so far.

I guess Arm never got converted. That port needs some love.

Or, deprecation.

src/hotspot/share/asm/register.hpp

tstuefe

Looks good to me. Thanks for working in my remarks.

tstuefe · 2024-06-03T17:34:43Z

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp

@@ -68,6 +68,8 @@

 #include <sys/types.h>

+static_assert(different_registers(zr, sp), "fucked");


Debugging remnant?

Ah, LOL! :-)

kimbarrett

Still looks good.

theRealAph · 2024-06-05T17:08:30Z

/integrate

openjdk · 2024-06-05T17:09:56Z

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

f73922b: 8333235: vmTestbase/nsk/jdb/kill/kill001/kill001.java fails with C1
5dcb7a6: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F
438121b: 8332785: Replace naked uses of UseSharedSpaces with CDSConfig::is_using_archive
d7d1afb: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE
7acfba2: 8327650: Test java/nio/channels/DatagramChannel/StressNativeSignal.java timed out
c5c0867: 8333252: C2: assert(assertion_predicate_has_loop_opaque_node(iff)) failed: must find OpaqueLoop* nodes
d85b0ca: 8332457: Examine startup overheads from JDK-8294961
326dbb1: 8312436: CompletableFuture never completes when 'Throwable.toString()' method throws Exception
9a8096f: 8330047: ASAN build error with gcc 13
6882b38: 8333590: UnmodifiableHeaders.toString() returns a value that represents empty headers
... and 251 more: https://git.openjdk.org/jdk/compare/39a55e97799b5328da85aaa66c8d23175b305691...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-06-05T17:10:01Z

@theRealAph Pushed as commit 9b3694c.

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

Andrew Haley added 3 commits November 7, 2023 14:50

Temp

e26e5bb

Better

5d1e6ce

8319822: Use a linear-time algorithm for assert_different_registers()

faeaac8

openjdk bot added the rfr Pull request is ready for review label Nov 10, 2023

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Nov 10, 2023

theRealAph marked this pull request as draft November 10, 2023 15:54

openjdk bot removed the rfr Pull request is ready for review label Nov 10, 2023

Bleurgh

d6fc55a

theRealAph marked this pull request as ready for review November 10, 2023 17:49

openjdk bot added the rfr Pull request is ready for review label Nov 10, 2023

Fix x86

1d0dd72

Cleanup, fix warning on Windows.

62e013a

Andrew Haley added 2 commits November 17, 2023 15:40

8319822: Use a linear-time algorithm for assert_different_registers()

d122057

8319822: Use a linear-time algorithm for assert_different_registers()

ef8457f

Review feedback.

a945d09

stefank approved these changes May 13, 2024

View reviewed changes

src/hotspot/share/asm/register.hpp Outdated Show resolved Hide resolved

src/hotspot/share/asm/register.hpp Show resolved Hide resolved

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

Update src/hotspot/share/asm/register.hpp

951277b

Co-authored-by: Stefan Karlsson <stefan.karlsson@oracle.com>

kimbarrett suggested changes May 29, 2024

View reviewed changes

Andrew Haley added 3 commits May 31, 2024 16:50

Review feedback

3b1f399

Review feedback

656a271

Merge branch 'clean' into different-regs

c9fc63d

kimbarrett approved these changes May 31, 2024

View reviewed changes

src/hotspot/share/asm/register.hpp Show resolved Hide resolved

tstuefe reviewed Jun 3, 2024

View reviewed changes

Andrew Haley added 5 commits June 3, 2024 15:19

Windows fix

c346555

Review feedback

80ca17c

Review feedback

4768170

Review feedback

df953ad

Kludge to fix build on Arm 32

eac03cc

tstuefe approved these changes Jun 3, 2024

View reviewed changes

Review feedback

ddfda06

kimbarrett approved these changes Jun 4, 2024

View reviewed changes

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

openjdk bot closed this Jun 5, 2024

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

		@@ -245,14 +253,35 @@ inline ReverseRegSetIterator<RegImpl> AbstractRegSet<RegImpl>::rbegin() {

		// Debugging support

		template<typename R, typename... Rx>

		@@ -68,6 +68,8 @@

		#include <sys/types.h>

		static_assert(different_registers(zr, sp), "fucked");

8319822: Use a linear-time algorithm for assert_different_registers() #16617

8319822: Use a linear-time algorithm for assert_different_registers() #16617

Conversation

theRealAph commented Nov 10, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Nov 10, 2023

openjdk bot commented Nov 10, 2023

mlbridge bot commented Nov 10, 2023 • edited Loading

Webrevs

TobiHartmann commented Nov 13, 2023

theRealAph commented Nov 13, 2023

stefank commented Nov 13, 2023

theRealAph commented Nov 15, 2023

dean-long commented Nov 15, 2023

shqking commented Nov 16, 2023

theRealAph commented Nov 17, 2023

stefank commented Nov 20, 2023 • edited Loading

stefank commented Nov 20, 2023

theRealAph commented Nov 27, 2023 • edited Loading

theRealAph commented Nov 27, 2023

bridgekeeper bot commented Dec 25, 2023

theRealAph commented May 10, 2024

theRealAph commented May 10, 2024

stefank left a comment

Choose a reason for hiding this comment

theRealAph commented May 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theRealAph commented May 31, 2024

kimbarrett left a comment

Choose a reason for hiding this comment

tstuefe commented Jun 1, 2024

theRealAph commented Jun 3, 2024

tstuefe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstuefe Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstuefe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimbarrett left a comment

Choose a reason for hiding this comment

theRealAph commented Jun 5, 2024

openjdk bot commented Jun 5, 2024

openjdk bot commented Jun 5, 2024

theRealAph commented Nov 10, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 10, 2023 •

edited

Loading

stefank commented Nov 20, 2023 •

edited

Loading

theRealAph commented Nov 27, 2023 •

edited

Loading

tstuefe Jun 3, 2024 •

edited

Loading