RFR: 8325467: Support methods with many arguments in C2 [v27]
Emanuel Peter
epeter at openjdk.org
Tue Sep 16 11:07:56 UTC 2025
On Tue, 16 Sep 2025 08:52:57 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 with a new target base due to a merge or a rebase. The pull request now contains 39 commits:
>
> - Clarify comments in regmask.hpp
> - Merge remote-tracking branch 'upstream/master' into many-arguments-8325467+pr-updates
> - Address review comments (renaming on the way in a separate PR)
> - Update src/hotspot/share/opto/regmask.hpp
>
> Co-authored-by: Emanuel Peter <emanuel.peter at oracle.com>
> - Restore modified java/lang/invoke tests
> - Sort includes (new requirement)
> - Merge remote-tracking branch 'upstream/master' into many-arguments-8325467+pr-updates
> - Add clarifying comments at definitions of register mask sizes
> - Fix implicit zero and nullptr checks
> - Add deep copy comment
> - ... and 29 more: https://git.openjdk.org/jdk/compare/60930a3e...c1f41288
A first batch of comments before lunch (made it half way through `regmask.hpp)`
src/hotspot/share/adlc/formsopt.cpp line 174:
> 172: // The array of Register Mask bits should be large enough to cover all the
> 173: // machine registers and usually all parameters that need to be passed on the
> 174: // stack (stack registers) up to some interesting limit. On Intel, the limit
What do you mean by `usually`? It could be misunderstood that sometimes it may not be large enough to even cover those "upt to some interesting limit". Consider rephrasing for clarity ;)
src/hotspot/share/opto/chaitin.cpp line 645:
> 643: if (C->failing()) {
> 644: return;
> 645: }
What can fail here?
src/hotspot/share/opto/chaitin.hpp line 151:
> 149: _mask_size = _mask.rm_size_in_bits();
> 150: return _mask.rollover();
> 151: }
Subjective: I would have kept the one-liner approach consistently here, since that is what surrounding code does.
src/hotspot/share/opto/ifg.cpp line 732:
> 730:
> 731: // Remove bound register(s) from 'l's choices
> 732: old = interfering_lrg.mask();
Just checking: This is an implicit `copy` case, right?
src/hotspot/share/opto/locknode.cpp line 43:
> 41:
> 42: BoxLockNode::BoxLockNode(int slot)
> 43: : Node(Compile::current()->root()), _slot(slot),
Suggestion:
: Node(Compile::current()->root()),
_slot(slot),
I would put all on separate lines. Optional.
src/hotspot/share/opto/locknode.cpp line 55:
> 53: }
> 54: init_class_id(Class_BoxLock);
> 55: init_flags(Flag_rematerialize);
Any reason why you moved these after the bailout? Maybe that's fine, but I don't know what the implications might be. Do you?
src/hotspot/share/opto/machnode.hpp line 758:
> 756: public:
> 757: MachProjNode(Node* multi, uint con, const RegMask& out, uint ideal_reg)
> 758: : ProjNode(multi, con), _rout(out, Compile::current()->comp_arena()),
Suggestion:
: ProjNode(multi, con),
_rout(out, Compile::current()->comp_arena()),
Optional. Either list horizontally or vertically is my opinion ;)
src/hotspot/share/opto/postaloc.cpp line 681:
> 679: for (int l = 1; l < n_regs; l++) {
> 680: OptoReg::Name ureg_lo = OptoReg::add(ureg,-l);
> 681: bool is_reg = OptoReg::is_reg(ureg_lo);
Only needed in assert. Do you really need to give it a separate name? Subjective, your choice.
Does it have a side-effect?
src/hotspot/share/opto/postaloc.cpp line 685:
> 683: assert(is_adjacent || is_reg,
> 684: "only registers can be non-adjacent");
> 685: if (!value[ureg_lo] && is_adjacent) { // Nearly always adjacent
`value[ureg_lo]` returns a `Node*`, right? Then that would make this an implicit null check, not allowed by style guide ;)
src/hotspot/share/opto/regmask.hpp line 122:
> 120: // the machine registers and usually all parameters that need to be passed
> 121: // on the stack (stack registers) up to some interesting limit. On Intel,
> 122: // the limit is something like 90+ parameters.
You may say that that in the "unusual" case, we have to use `_rm_word_ext`. Just so the reader knows what the ominous "usually" refers to ;)
src/hotspot/share/opto/regmask.hpp line 217:
> 215: // are included in the register mask. Depending on the value of
> 216: // _infinite_stack (denoted with as), {s10, s11, ...} are all included (as=1)
> 217: // or excluded (as=0). Note that all registers/stack locations under _lwm
Do you want to rename `as` now that it does not refer to `all_stack` but `infinite_stack`?
src/hotspot/share/opto/regmask.hpp line 267:
> 265:
> 266: // Where to extend the register mask
> 267: Arena* _arena;
Usually, we try to keep all fields at the top.
src/hotspot/share/opto/regmask.hpp line 270:
> 268:
> 269: // Grow the register mask to ensure it can fit at least min_size words.
> 270: void grow(unsigned int min_size, bool init = true) {
Suggestion:
void grow(unsigned int min_size, bool initialize_... = true) {
I would spell out what it means. `init` could mean lots of things.
src/hotspot/share/opto/regmask.hpp line 285:
> 283: assert(_original_ext_address == &_rm_word_ext, "clone sanity check");
> 284: _rm_word_ext = REALLOC_ARENA_ARRAY(_arena, uintptr_t, _rm_word_ext,
> 285: old_ext_size, new_ext_size);
Suggestion:
old_ext_size, new_ext_size);
src/hotspot/share/opto/regmask.hpp line 450:
> 448: Insert(reg);
> 449: }
> 450: RegMask(OptoReg::Name reg) : RegMask(reg, nullptr) {}
You may want to add `explicit`, so nobody accidentally converts them ;)
src/hotspot/share/opto/regmask.hpp line 458:
> 456: }
> 457:
> 458: RegMask(const RegMask& rm) : RegMask(rm, nullptr) {}
Do you want to add `explicit` here?
This is a shallow copy, right? Maybe add a comment for that.
src/hotspot/share/opto/regmask.hpp line 464:
> 462: copy(rm);
> 463: return *this;
> 464: }
You could also delete this one, and use the `copy` explicitly at the use site. That would make the allocations a bit more explicit. What do you think?
Whenever possible, it is nice to be able to declare a type `NONCOPYABLE`. Especially if it does allocations where copy is non-trivial.
src/hotspot/share/opto/regmask.hpp line 659:
> 657:
> 658: // Fill a register mask with 1's starting from the given register.
> 659: void Set_All_From(OptoReg::Name reg) {
Oh boy, we have a mixture of `lower_case` and `Strange_Case` method names.
We missed those in the renaming RFE :/
-------------
Changes requested by epeter (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20404#pullrequestreview-3228765757
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2351720167
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2351729163
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2351797677
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2351813653
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2351821908
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2351831266
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2351835412
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2351891004
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2351885171
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2351923440
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2351946036
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2351970184
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2351976938
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2351972811
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2351741579
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2352007574
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2351754230
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2351864011
More information about the hotspot-compiler-dev
mailing list