RFR: 8342156: C2: Compilation failure with fewer arguments after JDK-8329032

[Roberto Castañeda Lozano]

On Mon, 21 Oct 2024 14:05:54 GMT, Daniel Lundén <dlunden at openjdk.org> wrote:

> Adding C2 register allocation support for APX EGPRs ([JDK-8329032](https://bugs.openjdk.org/browse/JDK-8329032)) reduced, due to unfortunate rounding in the register mask size computation, the available space for incoming/outgoing method arguments in register masks.
> 
> ### Changeset
> 
> - Bump the number of 32-bit words dedicated to incoming/outgoing arguments in register masks from 3 to 4.
> - Add a regression test.
> 
> ### Testing
> 
> - [GitHub Actions](https://github.com/dlunde/jdk/actions/runs/11436050131)
> - `tier1` to `tier4` (and additional Oracle-internal testing) on Windows x64, Linux x64, Linux aarch64, macOS x64, and macOS aarch64.
> - C2 compilation time benchmarking for DaCapo on Windows x64, Linux x64, Linux aarch64, macOS x64, and macOS aarch64. There is no observable difference in C2 compilation time.

src/hotspot/share/adlc/formsopt.cpp line 177:

> 175:   // Add one more word to avoid problematic rounding. Specifically, APX added
> 176:   // 16 more registers but did not result in a mask size increase.
> 177:   // Round up to the next doubleword size.

I agree with Dean that the comment should state more clearly what is problematic with the current rounding. From your analysis in the JBS issue, I take it that the problem is that
1) the number of bits left for stack locations depends on the value of `words_for_regs`,
2) before the addition of APX registers we were relying on the slack created by the up-rounding to accommodate enough stack locations, and
3) after adding APX registers this slack has been reduced significantly.

Is this the case? If so, I suggest to update the comment to something like:

Suggestion:

  // Round up to the next doubleword size.
  // Add one more word to accommodate a reasonable number of stack locations
  // in the register mask regardless of how much slack is created by rounding up.

test/hotspot/jtreg/compiler/arguments/TestManyParameters.java line 26:

> 24: /**
> 25:  * @test
> 26:  * @requires os.arch=="amd64" | os.arch=="x86_64"

Suggestion:

 * @requires os.simpleArch == "x64"

test/hotspot/jtreg/compiler/arguments/TestManyParameters.java line 28:

> 26:  * @requires os.arch=="amd64" | os.arch=="x86_64"
> 27:  * @bug 8342156
> 28:  * @summary Check that C2 restriction on number of method arguments is not too

Suggestion:

 * @summary Check that C2's restriction on number of method arguments is not too

test/hotspot/jtreg/compiler/arguments/TestManyParameters.java line 43:

> 41: 
> 42:     public static void main(String[] args) {
> 43:         test(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54);

Is this the maximum number of arguments that C2 could handle before the addition of APX?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21612#discussion_r1810269683
PR Review Comment: https://git.openjdk.org/jdk/pull/21612#discussion_r1810227860
PR Review Comment: https://git.openjdk.org/jdk/pull/21612#discussion_r1810229060
PR Review Comment: https://git.openjdk.org/jdk/pull/21612#discussion_r1810232586