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

Wed Apr 23 07:15:54 UTC 2025

On Thu, 10 Apr 2025 09:44:14 GMT, Daniel Lundén <dlunden at openjdk.org> wrote:

>> If a method has a large number of parameters, we currently bail out from C2 compilation.
>> 
>> ### Changeset
>> 
>> Allowing C2 compilation of methods with a large number of parameters requires fundamental changes to the register mask data structure, used in many places in C2. In particular, register masks currently have a statically determined size and cannot represent arbitrary numbers of stack slots. This is needed if we want to compile methods with arbitrary numbers of parameters. Register mask operations are present in performance-sensitive parts of C2, which further complicates changes.
>> 
>> Changes:
>> - Add functionality to dynamically grow/extend register masks. I experimented with a number of design choices to achieve this. To keep the common case (normal number of method parameters) quick and also to avoid more intrusive changes to the current `RegMask` interface, I decided to leave the "base" statically allocated memory for masks unchanged and only use dynamically allocated memory in the rare cases where it is needed.
>> - Generalize the "chunk"-logic from `PhaseChaitin::Select()` to allow arbitrary-sized chunks, and also move most of the logic into register mask methods to separate concerns and to make the `PhaseChaitin::Select()` code more readable.
>> - Remove all `can_represent` checks and bailouts.
>> - Performance tuning. A particularly important change is the early-exit optimization in `RegMask::overlap`, used in the performance-sensitive method `PhaseChaitin::interfere_with_live`.
>> - Add a new test case `TestManyMethodArguments.java` and extend an old test `TestNestedSynchronize.java`.
>> 
>> ### Testing
>> 
>> - [GitHub Actions](https://github.com/dlunde/jdk/actions/runs/10178060450)
>> - `tier1` to `tier4` (and additional Oracle-internal testing) on Windows x64, Linux x64, Linux aarch64, macOS x64, and macOS aarch64.
>> - Standard performance benchmarking. No observed conclusive overall performance degradation/improvement.
>> - Specific benchmarking of C2 compilation time. The changes increase C2 compilation time by, approximately and on average, 1% for methods that could also be compiled before this changeset (see the figure below). The reason for the degradation is further checks required in performance-sensitive code (in particular `PhaseChaitin::remove_bound_register_from_interfering_live_ranges`). I have tried optimizing in various ways, but changes I found that lead to improvement also lead to less readable code (and are, in my opinion, no...
>
> Daniel Lundén has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Refactor and improve TestNestedSynchronize.java

Fought my way down to `src/hotspot/share/opto/regmask.hpp`. Need to hop off the train, so I'll push some comments already now. More later.

src/hotspot/share/opto/matcher.cpp line 1340:

> 1338:   // Empty them all.
> 1339:   for (uint i = 0; i < cnt; i++) {
> 1340:     ::new (&(msfpt->_in_rms[i])) RegMask(C->comp_arena());

Here you do array indexing with `&`. Above you did `base + i`. Just an observation, I leave it to you if you want to do something about that.

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

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?

src/hotspot/share/opto/regmask.hpp line 143:

> 141:   // constructors. If we get copied/cloned, &_RM_UP_EXT will no longer equal
> 142:   // orig_ext_adr.
> 143:   uintptr_t** orig_ext_adr = &_RM_UP_EXT;

There should also be an underscore for this field, for consistency, right?

src/hotspot/share/opto/regmask.hpp line 182:

> 180:   //   r0 r1 r2 r3 r4 r5 r6 r7 r8 r9 s0 s1   s2 s3 s4 s5 s6 s7 s8 s9 s10 s11 ...
> 181:   //  [0  0  0  0 |0  1  1  0 |0  0  1  0 ] [1  1  0  1 |0  0  0  0] as  as  as
> 182:   // [0]         [1]         [2]           [0]         [1]

I kinda missed these two lines at first... it is not directly clear what they are for.
The first one is the mask, aaah and it is extended with the `as` values.
No idea yet what the second line is about...

Maybe some annotation would help? For me it would ok if that means the lines become a little longer.

src/hotspot/share/opto/regmask.hpp line 193:

> 191:   // In this example, registers {r5, r6} and stack locations {s0, s2, s3, s5}
> 192:   // are included in the register mask. Depending on the value of _all_stack,
> 193:   // (s10, s11, ...) are all included (as = 1) or excluded (as = 0). Note that

Maybe I missed it: what is `as`?

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?

src/hotspot/share/opto/regmask.hpp line 219:

> 217: 
> 218:   // Access word i in the register mask.
> 219:   const uintptr_t& _rm_up(unsigned int i) const {

You have another set of underscore methods here... did you mean to make them private? I quickly checked, and did not find any underscore methods in the file before your change.

This looks a little like python style, where you cannot make a method private, and so the convention is to make them underscore :)

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

PR Review: https://git.openjdk.org/jdk/pull/20404#pullrequestreview-2786035319
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2055312723
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2055320209
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2055330115
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2055354470
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2055368753
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2055360352
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2055369508
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2055378718