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

[Emanuel Peter]

On Wed, 23 Apr 2025 07:57:15 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 448:
> 
>> 446:   }
>> 447: 
>> 448:   // Empty mask check. Ignores registers included through the all-stack flag.
> 
> Suggestion:
> 
>   // Empty mask check. Ignores registers included through the all_stack flag.
> 
> For "greppability"

Why is that ok to ignore the `all_stack` registers? Would that be expected/clear at the use-site?

> src/hotspot/share/opto/regmask.hpp line 551:
> 
>> 549:   static int num_registers(uint ireg, LRG &lrg);
>> 550: 
>> 551:   // Overlap test. Non-zero if any registers in common, including all-stack.
> 
> Suggestion:
> 
>   // Overlap test. Non-zero if any registers in common, including all_stack.
> 
> greppability

There are more, I won't tag them all now ;)

> src/hotspot/share/opto/regmask.hpp line 561:
> 
>> 559:     unsigned lwm = MAX2(_lwm, rm._lwm);
>> 560:     for (unsigned i = lwm; i <= hwm; i++) {
>> 561:       if (_rm_up(i) & rm._rm_up(i)) {
> 
> Implicit null check?

More cases below, won't tag them all now.

> src/hotspot/share/opto/regmask.hpp line 589:
> 
>> 587:         }
>> 588:       }
>> 589:     }
> 
> There is a bit of code duplication here. A helper method could help, no pun intended.

Boah, but it is also not horrible to leave as is. Maybe a helper method does not make more readable. Not sure.

> src/hotspot/share/opto/regmask.hpp line 799:
> 
>> 797: 
>> 798: public:
>> 799:   unsigned int static basic_rm_size() { return _RM_SIZE; }
> 
> What makes it `basic`? Elsewhere, you call `RM_SIZE` the "base size". I don't understand this part, so I'm just asking if you think it is consistent?

The name divergence between `basic_rm_size` and `_RM_SIZE` generally makes me a little suspicious if we chose the names right? Why do we even need the `RM` / `rm` prefix everyhwere? Is that not already clear from the context, we are after all in a register mask 😅  Not sure if it is worth changing everything now, or ever. But we should at least look for consistency ;)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2055476393
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2055489553
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2055495759
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2055494825
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2055522590