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

[Roberto Castañeda Lozano]

On Thu, 3 Apr 2025 12:48:31 GMT, Daniel Lundén <dlunden at openjdk.org> wrote:

>> src/hotspot/share/opto/matcher.cpp line 195:
>> 
>>> 193:   if (C->failing()) {
>>> 194:     return;
>>> 195:   }
>> 
>> Is this failure poll required after your changes?
>
> Yes, this poll is still required. We may fail in `init_spill_mask -> regmask_for_ideal_register`.

Good catch, thanks for checking.

>> src/hotspot/share/opto/postaloc.cpp line 686:
>> 
>>> 684:               assert(!(!value[ureg_lo] && lrgs(useidx).mask().is_offset() &&
>>> 685:                        !lrgs(useidx).mask().Member(ureg_lo)),
>>> 686:                      "invalid assumption");
>> 
>> Could you use more descriptive names and assertion messages in this new assertion and the one below? Ideally, without having to refer to old versions. What is the invariant that we want to check? How does it relate to the surrounding code?
>
> As we've previously discussed offline, I also had my doubts when introducing these asserts. I've now had a second look (with reasonably fresh eyes), and believe I now better understand the underlying assumptions.
> 
> The two problematic pieces of code in `postaloc.cpp` from before this changeset that we need to translate as part of the changeset are
> 
> if (!value[ureg_lo] &&
>     (!RegMask::can_represent(ureg_lo) ||
>      lrgs(useidx).mask().Member(ureg_lo))) { // Nearly always adjacent
> 
> and
> 
> if( RegMask::can_represent(nreg_lo) &&     // Either a spill slot, or
>     !lrgs(lidx).mask().Member(nreg_lo) ) { // Nearly always adjacent
> 
> Specifically, the `RegMask::can_represent` calls check if their argument registers can fit in the statically determined size of register masks (which no longer makes sense in this changeset).
> 
> The reason for the `can_represent` calls is that the subsequent `Member` calls assert internally that their arguments can fit within the static size of register masks. That is, `can_represent` worked as a guard to ensure the precondition for the call to `Member` holds. In this changeset, the `Member` function is generalized to allow arbitrary arguments (and the interal assert is removed). Therefore, we can remove the `can_represent` guards.
> 
> Now to the assertions that I added (which I've now improved). From the if conditions, we can infer there is an implicit invariant that a register for which `can_represent` returns false is necessarily "adjacent". Specifically, `can_represent` returning false implies that the register is a spill slot (implied by a comment in the source code). However, registers for which `can_represent` returns true may **also** be spill splots, so using `can_represent` as a proxy check for spill slots feels clumsy. I believe that the real invariant here is that only actual registers (and not stack locations, including spill slots) can be non-adjacent. This is what I now verify with my updated asserts.
> 
> For the record, I have not been able to find any cases with non-adjacency in any tests on current Oracle-supported platforms. From another comment in the source code, it looks like non-adjacent pairs are quite specific to SPARC.

Good analysis, thanks for investigating Daniel! Maybe worth creating an RFE to investigate whether we can assume (and statically verify) non-adjacent register pairs moving forward, and cleanup this and possibly other C2 back-end code accordingly.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2028478202
PR Review Comment: https://git.openjdk.org/jdk/pull/20404#discussion_r2028473435