RFR: 8325467: Support methods with many arguments in C2 [v17]

Fri Apr 25 18:26:46 UTC 2025

On Wed, 23 Apr 2025 08:00:05 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Daniel Lundén has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Refactor and improve TestNestedSynchronize.java
>
> src/hotspot/share/opto/regmask.hpp line 452:
> 
>> 450:     assert(valid_watermarks(), "sanity");
>> 451:     for (unsigned i = _lwm; i <= _hwm; i++) {
>> 452:       if (_rm_up(i)) {
> 
> Is this an implicit `nullptr` check? If so, you need to make it explicit, the style guide forbids implicit null checks.

This is a bit tricky. It is indeed a pointer type (`uintptr_t`), but it is not used as a pointer. It is used to store the register mask bits. So I guess this is then not an implicit null check? It is an implicit zero check :slightly_smiling_face:

> src/hotspot/share/opto/regmask.hpp line 464:
> 
>> 462:     for (unsigned i = _lwm; i <= _hwm; i++) {
>> 463:       uintptr_t bits = _rm_up(i);
>> 464:       if (bits) {
> 
> This also looks like an implicit null check.

Let's continue the discussion in the first comment (resolving this). Depending on what we decide, I'll fix all the (potential) implicit null checks.

> src/hotspot/share/opto/regmask.hpp line 473:
> 
>> 471: 
>> 472:   // Get highest-numbered register from mask, or BAD if mask is empty. Ignores
>> 473:   // registers included through the all-stack flag.
> 
> Suggestion:
> 
>   // registers included through the all_stack flag.
> 
> Greppability

Yes, all fixed

> src/hotspot/share/opto/regmask.hpp line 480:
> 
>> 478:     while (i > _lwm) {
>> 479:       uintptr_t bits = _rm_up(--i);
>> 480:       if (bits) {
> 
> Implicit null check?

Same here, resolving.

> src/hotspot/share/opto/regmask.hpp line 512:
> 
>> 510:       tmp |= _rm_up(i);
>> 511:     }
>> 512:     return !tmp && is_AllStack();
> 
> Implicit null checks?
> 
> Could you not check `is_AllStack` early and return already?

Thanks, I added an early exit for `is_AllStack` and also an early exit in the loop.

> src/hotspot/share/opto/regmask.hpp line 619:
> 
>> 617:     _hwm = _rm_max();
>> 618:     _set_range(0, 0xFF, _rm_size);
>> 619:     set_AllStack(true);
> 
> Suggestion:
> 
>     set_AllStack();
> 
> You already have a default `true` value, right? Check for other occurances.

Thanks, I just removed the default value and made it explicit everywhere.

> src/hotspot/share/opto/regmask.hpp line 747:
> 
>> 745: 
>> 746:   // Subtract 'rm' from 'this', but ignore everything in 'rm' that does not
>> 747:   // overlap with us and to not modify our all-stack flag. Supports masks of
> 
> A little confused about the grammar in `and to not modify our all-stack flag`...

Thanks, typo. Fixed "to" -> "do"

> test/hotspot/jtreg/compiler/arguments/TestMaxMethodArguments.java line 43:
> 
>> 41:         try {
>> 42:             test(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196, 197, 198, 199, 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210, 211, 212, 213, 214, 215, 216, 21
 7, 218, 219, 220, 221, 222, 223, 224, 225, 226, 227, 228, 229, 230, 231, 232, 233, 234, 235, 236, 237, 238, 239, 240, 241, 242, 243, 244, 245, 246, 247, 248, 249, 250, 251, 252, 253, 254, 255);
>> 43:         } catch (Exception e) {
> 
> That makes me a little nervous. You catch all exceptions. What if we throw some unexpected null pointer exception? Is that fine too?
> 
> Suggestion: define your own exception, and only catch that one.

Thanks, fixed!

> test/hotspot/jtreg/compiler/locks/TestNestedSynchronize.java line 90:
> 
>> 88:     // The above is a massive program. Therefore, we do not directly inline the
>> 89:     // program in TestNestedSynchronize and instead compile and run it via the
>> 90:     // CompileFramework.
> 
> Nice, I like it. Of course I have no bias here ;)

Great :slightly_smiling_face:

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2060686693
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2060688125
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2060688715
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2060689145
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2060690375
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2060692592
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2060695524
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2060697403
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2060698080