RFR: 8319822: Use a linear-time algorithm for assert_different_registers() [v6]

[Kim Barrett]

On Mon, 27 Nov 2023 12:57:31 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> 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.
>
> Andrew Haley has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review feedback.

Changes requested by kbarrett (Reviewer).

src/hotspot/cpu/aarch64/register_aarch64.hpp line 73:

> 71: 
> 72:   constexpr bool operator==(const Register r) const { return _encoding == r._encoding; }
> 73:   constexpr bool operator!=(const Register r) const { return _encoding != r._encoding; }

This seems unrelated to the rest of this change.  It also seems like something that should be done for all
of the register_<cpu> variants.

src/hotspot/cpu/x86/register_x86.hpp line 395:

> 393: inline Register AbstractRegSet<Register>::first() {
> 394:   size_t first = _bitset & -_bitset;
> 395:   return first ? as_Register(exact_log2(first)) : noreg;

pre-existing: violation of the "Avoid implicit conversions to bool" rule from the style guide.
Similarly for the XMMRegister case.

src/hotspot/share/asm/register.hpp line 163:

> 161:   }
> 162: 
> 163:   constexpr uint size() const { return population_count(_bitset); }

population_count is currently not constexpr.  I'm surprised this doesn't lead to warnings for guaranteed
non-constexpr body of a constexpr function.  I'm pretty sure I've seen such warnings from some compiler.
Note that I think there's no reason for it to not be constexpr, other than preceding our use of C++11/14 and
not yet updated accordingly.  https://bugs.openjdk.org/browse/JDK-8323952

src/hotspot/share/asm/register.hpp line 257:

> 255: 
> 256: template<typename R, typename... Rx>
> 257: inline constexpr bool different_registers(AbstractRegSet<R> allocated_regs, R first_register) {

"inline" is redundant with "constexpr".

src/hotspot/share/asm/register.hpp line 257:

> 255: 
> 256: template<typename R, typename... Rx>
> 257: inline constexpr bool different_registers(AbstractRegSet<R> allocated_regs, R first_register) {

different_registers is only used by debug-only code in assert_different_registers.  Shouldn't all the overloads
for different_registers be within an `#ifdef ASSERT` block?

src/hotspot/share/asm/register.hpp line 273:

> 271: }
> 272: 
> 273: template<typename R, typename... Rx>

Rx is unused and not needed.  Similarly for 3-R overload.

src/hotspot/share/asm/register.hpp line 281:

> 279: inline constexpr bool different_registers(R reg1, R reg2, R reg3) {
> 280:   return reg1 != reg2 && reg2 != reg3 && reg1 != reg3;
> 281: }

The 2-R and 3-R overloads are just an optimization (probably) of the variadic-R overload for small numbers
of arguments.  Given this is debug-only code, is it really worth the additional source code?

-------------

PR Review: https://git.openjdk.org/jdk/pull/16617#pullrequestreview-1826587710
PR Review Comment: https://git.openjdk.org/jdk/pull/16617#discussion_r1454772535
PR Review Comment: https://git.openjdk.org/jdk/pull/16617#discussion_r1454736036
PR Review Comment: https://git.openjdk.org/jdk/pull/16617#discussion_r1454856106
PR Review Comment: https://git.openjdk.org/jdk/pull/16617#discussion_r1454760813
PR Review Comment: https://git.openjdk.org/jdk/pull/16617#discussion_r1454812257
PR Review Comment: https://git.openjdk.org/jdk/pull/16617#discussion_r1454796895
PR Review Comment: https://git.openjdk.org/jdk/pull/16617#discussion_r1454808568