✨ อัปเดตล่าสุด: eclipse omr/omr/pull/

ehsankianifar · 2024-01-16T18:02:23Z

If the addresses of two arrays are equal, those two arrays are considered equal and we do not need to actually compare those arrays. In this PR, we modified the instructions for x86 and there was a discussion to do the same for other architectures. On Z, it is more complicated and depending on the conditions and expected response, several methods implement array compare.
This PR targets array cmp helper implementation when the length is variable. the arraycmpEvaluator calls arraycmpHelper when the length is variable or larger than 4k and requested response is unsigned.

ehsankianifar · 2024-01-16T18:03:11Z

Instruction selection for this path:

 [     0x2aa17d56570]	                       LG      &GPR_0018, Parm[Parm  0<parm 0 L>] ?+0(GPR15)
 [     0x2aa17d743b0]	                       LG      &GPR_0020, Parm[Parm  1<parm 1 L>] ?+8(GPR15)
 [     0x2aa17d746f0]	                       L       GPR_0024, Parm[Parm  2<parm 2 I>] ?+16(GPR15)
 [     0x2aa17d748c0]	                       XGR     GPR_0017,GPR_0017	
 [     0x2aa17d74970]	                       Label L0016:	# (Start of internal control flow)	
 [     0x2aa17d74a30]	                       CLGRJ   &GPR_0018,&GPR_0020,Label L0017,BH(mask=0x8), 	
 [     0x2aa17d74b70]	                       AHI     GPR_0024,0xffff	
 [     0x2aa17d74c30]	                       BRC     BM(0x4), Label L0017	
 [     0x2aa17d942f0]	                       LR      GPR_0025,GPR_0024	
 [     0x2aa17d943a0]	                       SRA     GPR_0025,8
 [     0x2aa17d94460]	                       BRC     BH(0x8), Label L0019	
 [     0x2aa17d946a0]	                       Label L0018:	
 [     0x2aa17d94760]	                       CLC     #390 0(256,&GPR_0018),#391 0(&GPR_0020)
 [     0x2aa17d94820]	                       BRC     MASK7(0x6), Label L0020	
 [     0x2aa17d949a0]	                       LA      &GPR_0018,#392 256(&GPR_0018)
 [     0x2aa17d94b10]	                       LA      &GPR_0020,#393 256(&GPR_0020)
 [     0x2aa17d94bc0]	                       BRCT    GPR_0025,Label L0018	
 [     0x2aa17d94c80]	                       Label L0019:	
 [     0x2aa17d95030]	                       LARL    GPR_0026, &<LiteralPool Base Address>	 ; LoadLitPool
 [     0x2aa17db4a00]	                       assocreg
 [     0x2aa17db4470]	                       EX      GPR_0024,#396 0(GPR_0026)
 POST:
 {AssignAny:GPR_0017:R} {AssignAny:&GPR_0018:R} {AssignAny:&GPR_0020:R} {AssignAny:GPR_0024:R} {AssignAny:GPR_0025:R} {AssignAny:GPR_0026:R}
 [     0x2aa17db4ab0]	                       Label L0020:	
 [     0x2aa17db4b70]	                       BRC     MASK9(0x8), Label L0017	
 [     0x2aa17db4c30]	                       LGHI    GPR_0017,0x2	
 [     0x2aa17db4cf0]	                       BRC     MASK3(0x2), Label L0017	
 [     0x2aa17db4db0]	                       LGHI    GPR_0017,0x1	
 [     0x2aa17db5530]	                       assocreg
 [     0x2aa17db4f90]	                       Label L0017:	
 POST:
 {AssignAny:GPR_0017:R} {AssignAny:&GPR_0018:R} {AssignAny:&GPR_0020:R} {AssignAny:GPR_0024:R} {AssignAny:GPR_0025:R} {AssignAny:GPR_0026:R}
 [     0x2aa17db5690]	                       LGFR    GPR_0017,GPR_0017	
 [     0x2aa17db5cf0]	                       assocreg
 [     0x2aa17db5740]	                       retn    	
 POST:
 {GPR2:GPR_0017:R}

ehsankianifar · 2024-01-16T18:04:16Z

+      if (!isFoldedIf && needResultReg)
+         {
+         branchDeps->addPostCondition(retValReg, TR::RealRegister::AssignAny);
+         generateRRInstruction(cg, TR::InstOpCode::getXORRegOpCode(), node, retValReg, retValReg);


in all code paths we need to init result register with zero

ehsankianifar · 2024-01-16T18:05:01Z

+         generateRRInstruction(cg, TR::InstOpCode::getXORRegOpCode(), node, retValReg, retValReg);
+         }
+
+      generateS390LabelInstruction(cg, TR::InstOpCode::label, node, cFlowRegionStart);


The short circuit branch would always be the start of control flow.

ehsankianifar · 2024-01-16T18:06:19Z


      if (maxLenIn256)
         {
-         if (!isFoldedIf && needResultReg)


we init result before short circuit. this code no longer needed.

ehsankianifar · 2024-01-16T18:07:09Z

               {
               if (isIfxcmpBrCondContainEqual)
-                  setStartInternalControlFlow(cg, node, generateS390BranchInstruction(cg, TR::InstOpCode::BRC, TR::InstOpCode::COND_BL, node, compareTarget),
-                                              isStartInternalControlFlowSet);


control flow always start before short circuit branch. This is no longer necessary.

ehsankianifar · 2024-01-16T18:12:44Z

@r30shah Please review and let me know if it looks good to you.

ehsankianifar · 2024-01-17T14:47:20Z

Tested for JDK8 (LoZ and zOS) Axxon 64179, test level.sanity, all tests pass
JDK11 (LoZ) Build #20215, test sanity.functional sanity.system sanity.openjdk, all tests pass.

ehsankianifar · 2024-01-22T13:33:47Z

+         source1Reg,
+         source2Reg,
+         TR::InstOpCode::COND_BE,
+         isIfxcmpBrCondContainEqual ? compareTarget : cFlowRegionEnd);


set needCC to false.

ehsankianifar · 2024-01-23T18:50:26Z

After fixing jump instruction:
Tested for JDK8 (LoZ and zOS) Axxon 64612, test level.sanity, all tests pass
JDK11 (LoZ) Build #20268, test sanity.functional sanity.system sanity.openjdk, all tests pass.

r30shah

LGTM

r30shah · 2024-01-24T14:05:30Z

Jenkins build zos,zlinux

r30shah · 2024-01-25T02:22:23Z

@hzongaro Can I request you to merge this change if you are ok with it?

hzongaro

I think the changes look good. Thanks!

hzongaro · 2024-02-01T04:19:22Z

x86-64 macOS failure is unrelated, of course, while relevant platform-specific testing passed. Merging.

hzongaro · 2024-02-01T04:30:50Z

x86-64 macOS failure is unrelated, of course, while relevant platform-specific testing passed. Merging.

Actually, may I ask you to adjust the wording of the commit message to describe the change that's being applied? Maybe something like the following?

Add short circuit test of whether two array addresses are equal for
arraycmp.  This particular case handles unsigned result and variable
length or length greater than 4K.
Short circuit branching is added to the start of instruction sequence.

ehsankianifar · 2024-02-01T15:31:09Z

Thanks @hzongaro for your help. I modified the commit message as you suggested. Please let me know if it looks good now.

hzongaro · 2024-02-01T16:39:18Z

@ehsankianifar, thanks for updating the commit message. I don't know whether you've seen the Commit guidelines, but one thing they recommend is that lines in the body of the commit message shouldn't be longer than 72 characters, if possible. May I ask you to adjust the spacing so that each line is no longer than 72 characters?

Add short circuit test of whether two array addresses are equal for arraycmp. This particular case handles unsigned result and variable length or length greater than 4K. Short circuit branching is added to the start of instruction sequence. Signed-off-by: Ehsan Kiani Far <ehsan.kianifar@gmail.com>

ehsankianifar · 2024-02-01T17:26:02Z

Thanks @hzongaro for reminding me of that. I fixed the commit message.

ehsankianifar requested a review from fjeremic as a code owner January 16, 2024 18:02

ehsankianifar commented Jan 16, 2024

View reviewed changes

github-actions Bot added arch:z comp:compiler labels Jan 16, 2024

ehsankianifar commented Jan 22, 2024

View reviewed changes

ehsankianifar force-pushed the ShortCircuitVarLengthArrayCmpHelper branch 2 times, most recently from a535d71 to 0b82978 Compare January 23, 2024 15:00

r30shah approved these changes Jan 24, 2024

View reviewed changes

hzongaro self-assigned this Feb 1, 2024

hzongaro approved these changes Feb 1, 2024

View reviewed changes

ehsankianifar force-pushed the ShortCircuitVarLengthArrayCmpHelper branch from 0b82978 to 1f57d12 Compare February 1, 2024 15:28

ehsankianifar force-pushed the ShortCircuitVarLengthArrayCmpHelper branch 2 times, most recently from 86feda1 to 049a571 Compare February 1, 2024 17:22

ehsankianifar force-pushed the ShortCircuitVarLengthArrayCmpHelper branch from 049a571 to 47bc735 Compare February 1, 2024 17:24

hzongaro merged commit 15e19bf into eclipse-omr:master Feb 1, 2024

ehsankianifar mentioned this pull request Feb 21, 2024

WIP: Z: Add array cmp short circuit #7132

Closed

pshipton mentioned this pull request Feb 28, 2024

jdk_nio_0_FAILED sun/nio/cs/TestStringCoding.java RuntimeException: getBytes(csn) failed -> x-IBM29626C eclipse-openj9/openj9#18974

Closed

ehsankianifar deleted the ShortCircuitVarLengthArrayCmpHelper branch March 1, 2024 21:01

Uh oh!

Conversation

ehsankianifar commented Jan 16, 2024

Uh oh!

ehsankianifar commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ehsankianifar Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

ehsankianifar Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

ehsankianifar Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

ehsankianifar Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

ehsankianifar commented Jan 16, 2024

Uh oh!

ehsankianifar commented Jan 17, 2024

Uh oh!

ehsankianifar Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

ehsankianifar commented Jan 23, 2024

Uh oh!

r30shah left a comment

Choose a reason for hiding this comment

Uh oh!

r30shah commented Jan 24, 2024

Uh oh!

r30shah commented Jan 25, 2024

Uh oh!

hzongaro left a comment

Choose a reason for hiding this comment

Uh oh!

hzongaro commented Feb 1, 2024

Uh oh!

hzongaro commented Feb 1, 2024

Uh oh!

ehsankianifar commented Feb 1, 2024

Uh oh!

hzongaro commented Feb 1, 2024

Uh oh!

ehsankianifar commented Feb 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ehsankianifar commented Jan 16, 2024 •

edited

Loading