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

[Daniel Lundén]

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

>> src/hotspot/share/opto/node.cpp line 558:
>> 
>>> 556:     MachProjNode* mach = n->as_MachProj();
>>> 557:     MachProjNode* mthis = this->as_MachProj();
>>> 558:     new (&mach->_rout) RegMask(mthis->_rout);
>> 
>> Hmm. Do you have some defense against these kinds of shallow copies? Or are there cases where shallow copies of a RegMask are somehow valid? You could for example `nullptr` our the pointers in the copy constructor. Just an idea, not sure if that even works. Or you could do a proper copy in the copy constructor. Maybe you have already thought about both of those options, and neither are good...
>
> Ah, I see you have a `uintptr_t** orig_ext_adr = &_RM_UP_EXT;` trick. So shallow copies should not ever get used, right?

Yes, I had a lot of headache related to this because register masks often get cloned in unpredictable ways (without the use of a constructor). Correct, the `orig_ext_adr` (now `original_ext_address`) enables us to trigger asserts whenever we try to mutate a cloned register mask. We can allow the use of shallow copies if both the original and copy are immutable (only happens in one case at the moment, for `BoxLockNode` register masks). This is the assert `assert(_read_only || _original_ext_address == &_RM_UP_EXT, "clone sanity check");`

>> src/hotspot/share/opto/regmask.cpp line 395:
>> 
>>> 393: 
>>> 394: #ifndef PRODUCT
>>> 395: bool RegMask::_dump_end_run(outputStream* st, OptoReg::Name start,
>> 
>> In my understanding we only use underscore for fields, and not methods?
>
> I think just making it private is sufficient, no?

Thanks, removed the underscore (and it is already private).

>> src/hotspot/share/opto/regmask.hpp line 199:
>> 
>>> 197:   //
>>> 198:   // The only operation that may update the _offset attribute is
>>> 199:   // RegMask::rollover(). This operation requires the register mask to be clean
>> 
>> What does `clean` mean? All zero? Or all ones?
>
> I think all ones. You could write `... mask to be clean (all ones) and ...`

It actually means all zeroes (i.e., empty). The rollover operation is for a very specific use case in the register allocator where we have exhausted every possible register/stack location in an all-stack register mask, and "roll it over" to expose a new set of stack locations to the register allocator (this is the now moved `chunk` logic in `Select` that I mentioned in an earlier comment). Updated now! Note that there is also a comment next to the definition of the `rollover` method.

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

Thanks, I've changed all occurrences of `all-stack` to `all_stack`.

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

That's how it is expected to work. I had the same thought as you when working on this and changed it to not ignore all stack registers, but that broke a lot of things. It is not obvious why it works this way at all call sites. For some call sites, it is probably fine to also consider the all-stack flag (but not for all call sites).

>> test/jdk/java/lang/invoke/BigArityTest.java line 38:
>> 
>>> 36:  *                                 -XX:CompileCommand=memlimit,*.*,0
>>> 37:  *                                 -esa -DBigArityTest.ITERATION_COUNT=1
>>> 38:  *                                 test.java.lang.invoke.BigArityTest
>> 
>> Would it make sense to also have a run with fewer flags?
>
> Or why do we need to set all these flags? If we need some of them, then a comment could be helpful.

Note that I only added the `-XX:MaxNodeLimit=20000`, all the other flags are from before (I just added line breaks). It could very well make sense to have a run with fewer flags, but I'm not sure if that's compatible with what the original author intended. I'd prefer leaving it as it is.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2060668670
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2060674662
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2060676342
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2060682057
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2060667493