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

[Thomas Stuefe]

On Fri, 31 May 2024 16:02:40 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 19 additional commits since the last revision:
> 
>  - Merge branch 'clean' into different-regs
>  - Review feedback
>  - Review feedback
>  - Update src/hotspot/share/asm/register.hpp
>    
>    Co-authored-by: Stefan Karlsson <stefan.karlsson at oracle.com>
>  - Review feedback
>  - Review feedback
>  - Review feedback
>  - Merge branch 'different-regs' of https://github.com/theRealAph/jdk into different-regs
>  - Update src/hotspot/share/asm/register.hpp
>    
>    Co-authored-by: Emanuel Peter <emanuel.peter at oracle.com>
>  - Merge branch 'clean' into different-regs
>  - ... and 9 more: https://git.openjdk.org/jdk/compare/8fe7e1ce...c9fc63d7

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

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

> 94: template <class RegImpl>
> 95: class AbstractRegSet {
> 96:   size_t _bitset;

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

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

> 107: 
> 108:   constexpr AbstractRegSet(RegImpl r1)
> 109:     : _bitset(r1->is_valid() ? size_t(1) << r1->encoding() : 0) { }

If my assumption from above is correct, we never noticed ppc being broken since _bitset would have been 0 if encoding > 31.

Could you add something like:


STATIC_ASSERT(RegImpl::number_of_registers <= 64);


?

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

PR Review: https://git.openjdk.org/jdk/pull/16617#pullrequestreview-2093240269
PR Review Comment: https://git.openjdk.org/jdk/pull/16617#discussion_r1624068958
PR Review Comment: https://git.openjdk.org/jdk/pull/16617#discussion_r1624084824