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

[Daniel Lundén]

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

>> 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 ;)

All fixed 👍

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

Same here, resolving.

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

I see your point, but I'm not sure how much it helps readability. I suggest leaving it as is.

>> 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 ;)

Yes, it is confusing but consistent. Your intuition is correct: there is a difference between `_rm_size` (the current total size, including extension) and `_RM_SIZE` (the base static size) 😅. @robcasloz introduced the "basic" terminology when working on tests in `test_regmask.cpp` and needed some way to expose `_RM_SIZE` publically in non-product code. Therefore, we have the method `basic_rm_size`. I don't really have a better suggestion. Perhaps `base_rm_size`, or `static_rm_size`? As in "the base/static part of rm_size". We cannot call the method `_RM_SIZE()` as that is prohibited by the style guide. We cannot call the method `RM_SIZE()` as `RM_SIZE` is a macro (and also not the same thing as `_RM_SIZE` on 64-bit machines).

> Why do we even need the RM / rm prefix everyhwere?

We really don't, but that's how it is :slightly_smiling_face: Could be worth refactoring, but not in this changeset!

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2060690749
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2060691115
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2060691968
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2060696987