Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow multianewarray for 1D arrays on Z #17714

Merged
merged 1 commit into from
Jul 1, 2023

Conversation

JamesKingdon
Copy link
Contributor

The existing multianewarray evaluator assumes at least 2 dimensions which is consistent with normal usage. However the spec permits 1D arrays and this is exploited by some languages, e.g. Groovy.

See #17360 for more background and #17358 for the corresponding change on X

@JamesKingdon JamesKingdon marked this pull request as draft June 29, 2023 18:13
@JamesKingdon
Copy link
Contributor Author

JamesKingdon commented Jun 29, 2023

Personal build 17174 passed sanity.functional

I used a test case with asm to exercise multianewarray for a 1D array and logged the compile. The following line in the log indicates the new code is active:

Disabling inline allocations for multianewarray of dim 1

Bytecode for the test method:

        0, JBbipush                                  10
        2, JBmultianewarray                       3                      1
        6, JBastore0
        7, JBaload0
        8, JBreturn1

The generated code was:

<instructions
        title="Mixed Mode Disassembly"
        method="AsmTestClass.doTest()Ljava/lang/Object;">


 \\ AsmTestClass.doTest()Ljava/lang/Object;
 \\    0 JBbipush 10

     0x3ff5e5073f8 fffffffc [     0x3ff5d511d50]                                                   Label L0016:
     0x3ff5e5073f8 fffffffc [     0x3ff5d4b2900] 00 00 00 05                                       .dd     0x00000005
     0x3ff5e5073fc 00000000 [     0x3ff5d4b29d0]                                                   proc
     0x3ff5e5073fc 00000000 [     0x3ff5d512070] e3 e0 5f f8 ff 24                                 STG     GPR14,#401 -8(GPR5)
     0x3ff5e507402 00000006 [     0x3ff5d512200] e3 50 5f d8 ff 71                                 LAY     GPR5,#402 -40(GPR5)
     0x3ff5e507408 0000000c [     0x3ff5d512390] e3 50 d0 50 00 21                                 CLG     GPR5,#403 80(GPR13)
     0x3ff5e50740e 00000012 [     0x3ff5d512520] a7 44 00 1e                                       BRC     BM(0x4), Snippet Label L0018, labelTargetAddr=0x000003FF5E50744A
     0x3ff5e507412 00000016 [     0x3ff5d512700]                                                   Label L0019:
     0x3ff5e507412 00000016 [     0x3ff5d512ee0]                                                   assocreg
     0x3ff5e507412 00000016 [     0x3ff5d5128a0] e5 48 50 18 00 00                                 MVGHI   #404 24(GPR5),0x0000
     0x3ff5e507418 0000001c [     0x3ff5d4b2af0]                                                   fence   Relative [ 000003FF5D44E0E0 ] BBStart <block_2> (frequency 10000)
     0x3ff5e507418 0000001c [     0x3ff5d4b2e50] e5 4c 50 14 00 0a                                 MVHI     Auto[<auto slot -1>] 20(GPR5),0x000a

 \\ AsmTestClass.doTest()Ljava/lang/Object;
 \\    2 JBmultianewarray 3 dims 1 [Ljava/lang/Class;

     0x3ff5e50741e 00000022 [     0x3ff5d4b42b0] c0 1f 00 09 b4 00                                 LLILF   GPR1,635904
     0x3ff5e507424 00000028 [     0x3ff5d4b43c0] c0 18 00 00 00 00                                 IIHF    GPR1,0
     0x3ff5e50742a 0000002e [     0x3ff5d4b4540] a7 28 00 01                                       LHI     GPR2,0x1
     0x3ff5e50742e 00000032 [     0x3ff5d4b4710] 41 30 50 14                                       LA      GPR3, Auto[<auto slot -1>] 20(GPR5)
     0x3ff5e507432 00000036 [     0x3ff5d50ef30]                                                   assocreg
     0x3ff5e507432 00000036 [     0x3ff5d50e940] c0 e5 13 a3 d8 a7                                 BRASL   GPR14,0x0000000000000000, targetAddr=0x000003FF85982580 (offset=0x000000002747B14E)

 \\ AsmTestClass.doTest()Ljava/lang/Object;
 \\    6 JBastore0

     0x3ff5e507438 0000003c [     0x3ff5d50fa20] e3 20 50 18 00 24                                 STG     GPR2, Auto[<auto slot 0>] 24(GPR5)
     0x3ff5e50743e 00000042 [     0x3ff5d510340]                                                   assocreg

 \\ AsmTestClass.doTest()Ljava/lang/Object;
 \\    8 JBreturn1

     0x3ff5e50743e 00000042 [     0x3ff5d5131f0] e3 e0 50 20 00 04                                 LG      GPR14,#405 32(GPR5)
     0x3ff5e507444 00000048 [     0x3ff5d513440] 41 50 50 28                                       LA      GPR5,#407 40(GPR5)

 \\ AsmTestClass.doTest()Ljava/lang/Object;
 \\    6 JBastore0

     0x3ff5e507448 0000004c [     0x3ff5d513510] 07 fe                                             BCR     BER(mask=0xf), GPR14
     0x3ff5e50744a 0000004e [     0x3ff5d50fd70]                                                   retn
     0x3ff5e50744a 0000004e [     0x3ff5d5104d0]                                                   fence   Relative [ 000003FF5D44E0E4 ] BBEnd </block_2>

     0x3ff5e50744a 0000004e [     0x3ff5d510aa0]                                                   assocreg
     0x3ff5e50744a 0000004e [     0x3ff5d511e90]                                                   Label L0017:
</instructions>

@JamesKingdon JamesKingdon marked this pull request as ready for review June 29, 2023 18:40
}
else
{
// trace a message to indicate that inline allocation is disabled for nDims < 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a comment here? I think text in the Assert message is clear about disabling the inline allocation for multianewarray and also the comment on function makes it clear that for <1 dimensional array it calls helper.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, deleted

Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @JamesKingdon for fixing this on Z. Changes looks good to me. Left a comment about small nit-pick.

@joransiu can I request you to launch test on this PR and merge this ?

Copy link
Member

@joransiu joransiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I agree the comment Rahil highlighted is unnecessary.

The existing multianewarray evaluator assumes at least 2 dimensions
which is consistent with normal usage. However the spec permits 1D
arrays and this is exploited by some languages, e.g. Groovy.
@joransiu
Copy link
Member

Jenkins test sanity zlinux jdk11,jdk17

@joransiu joransiu merged commit 7c0f636 into eclipse-openj9:master Jul 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants