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

[Stefan Karlsson]

On Fri, 10 May 2024 15:26:29 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

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

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

> 99: 
> 100:   static constexpr int max_size() {
> 101:     return (int)(sizeof _bitset * CHAR_BIT);

This makes me have to think about operator precedence and what CHAR_BIT is (not typically used in HotSpot). I'd prefer to see something like this:
Suggestion:

    return (int)(sizeof(_bitset) * BitsPerByte);

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

> 261: template<typename R, typename... Rx>
> 262: inline constexpr bool different_registers(AbstractRegSet<R> allocated_regs, R first_register, Rx... more_registers) {
> 263:   if (allocated_regs.contains(first_register)) {

FWIW, while first reading this I was looking for the base case of the recursion (the previous versions had some extra specializations). To me it looks like the base case is written in both this function and the function above. I would prefer to have the implementation inside one function only and change this function to use:

  if (!different_registers(allocated_regs, first_register)) {

I think this could make it a bit clearer, but if you prefer the current style, I think that's fine as well.

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

Marked as reviewed by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16617#pullrequestreview-2052702736
PR Review Comment: https://git.openjdk.org/jdk/pull/16617#discussion_r1598475883
PR Review Comment: https://git.openjdk.org/jdk/pull/16617#discussion_r1598591701