Optimize primitive unboxing method calls

[jmesyou]

Boxed primitive classes have a method that unboxes their value, which are implemented as simple getter methods. These methods are marked as @IntrinsicCandidates, intended for simplification by the compiler.

This commit recognizes the primitive unboxing methods and generates simplified IL during IL generation instead of emitting the call where possible.

[0xdaryl]

Doing it this way will still consume inliner budget if I'm not mistaken (@nbhuiyan, please keep me honest).

I wonder if it would be better to recognize and inline these simple methods during ILGen by genInvoke() instead (see plenty of other cases in that function where we do this). That way, the inliner never has to consider them at all.

openj9/runtime/compiler/ilgen/Walker.cpp

Line 3519 in b669217

TR_J9ByteCodeIlGenerator::genInvoke(TR::SymbolReference * symRef, TR::Node *indirectCallFirstChild, TR::Node *invokedynamicReceiver)

[nbhuiyan]

Yes, methods under alwaysWorthInlining would have their inlineable status protected through most conditions that would have otherwise eliminated them, such as when the callee size exceeds some threshold. @jmesyou , if by reducing load from the inliner you meant to have these methods not consume inlining budget, then each of these methods would need special handling in TR_MultipleCallTargetInliner::weighCallSite or EstimateCodeSize to adjust their size/weight and not compete with other methods for the inlining budget. Preferably, if these can be handled in genInvoke as Daryl suggested, then none of that will be necessary.

[jmesyou]

I was able to move this optimization into the ILGen phase during genInvoke. Instead of emitting the method call during ILGen, if the known field for the boxed value can be found in the class block, a corresponding load of the field and null check of the receiver is emitted instead of the call. If the field doesn't exist for whatever reason, the implementation changes in the future, the call is emitted instead.

[hzongaro]

I think the changes look good overall. Just one minor issue that trips up some compilers.

[hzongaro]

Oops! Accidentally approved when I hadn't intended to.

[jmesyou]

@hzongaro ready for another pass

[hzongaro]

I think the changes look good - just one cosmetic change request. Otherwise approved. Thanks!

[hzongaro]

Jenkins test sanity all jdk8,jdk11,jdk17,jdk21

[hzongaro]

Looks good.

[hzongaro]

Looks like at least some of the build failures are real. For example, from the JDK 11 PPC64LE build:

17:57:09  __kernel_sigtramp_rt64+0x0 (0x0000100000050478)
17:57:09  _ZN9TR_Memory19allocateStackMemoryEmN13TR_MemoryBase10ObjectTypeE+0x28 (0x00001000016CAF58 [libj9jit29.so+0x6daf58])
17:57:09  _ZN15TR_VMFieldsInfoC1EPN2TR11CompilationEP7J9Classi17TR_AllocationKind+0x114 (0x0000100001217C84 [libj9jit29.so+0x227c84])
17:57:09  _ZN2J98ClassEnv15enumerateFieldsERN2TR6RegionEP19TR_OpaqueClassBlockPNS1_11CompilationE+0x508 (0x0000100001214098 [libj9jit29.so+0x224098])
17:57:09  _ZN3OMR11Compilation10typeLayoutEP19TR_OpaqueClassBlock+0xc4 (0x00001000016B5334 [libj9jit29.so+0x6c5334])
17:57:09  _ZL21createLoadFieldSymRefPN2TR11CompilationEP19TR_OpaqueClassBlockPKcb+0x50 (0x00001000012FC740 [libj9jit29.so+0x30c740])
17:57:09  _ZN24TR_J9ByteCodeIlGenerator9genInvokeEPN2TR15SymbolReferenceEPNS0_4NodeES4_+0x1d14 (0x0000100001317AA4 [libj9jit29.so+0x327aa4])
17:57:09  _ZN24TR_J9ByteCodeIlGenerator16genInvokeVirtualEi+0x154 (0x000010000131C624 [libj9jit29.so+0x32c624])
17:57:09  _ZN24TR_J9ByteCodeIlGenerator6walkerEPN2TR5BlockE+0x2594 (0x0000100001323244 [libj9jit29.so+0x333244])
17:57:09  _ZN24TR_J9ByteCodeIlGenerator18genILFromByteCodesEv+0x3f8 (0x00001000012F27A8 [libj9jit29.so+0x3027a8])
17:57:09  _ZN24TR_J9ByteCodeIlGenerator13internalGenILEv+0x288 (0x00001000012F4788 [libj9jit29.so+0x304788])
17:57:09  _ZN24TR_J9ByteCodeIlGenerator5genILEv+0x78 (0x00001000012F4BD8 [libj9jit29.so+0x304bd8])

[jmesyou]

Internal build #20439 passing after a check was inserted to check that loading a class from a signature did not result in NULL. Also, this optimization can now be disabled by TR_DisableIntrinsics

[hzongaro]

I think the changes look correct. Just a couple of minor comments.

[hzongaro]

Looks good! Thanks for the changes.

[hzongaro]

@0xdaryl, if you're satisfied with the changes, I'll run PR testing.

[hzongaro]

Jenkins test sanity all jdk8,jdk11,jdk17,jdk21

[hzongaro]

@jmesyou, it looks like there are some test failures for JDK 8, JDK 17 and JDK 21, all on Windows. May I ask you to investigate?

The JDK 21 AIX test build failure appears to be infrastructure-related.

[hzongaro]

@jmesyou, it looks like there are some test failures for JDK 8, JDK 17 and JDK 21, all on Windows. May I ask you to investigate?

I strongly suspect these failures might have been caused by the uncoordinated merges of #18876 and eclipse-omr/omr#7247 that I performed the same day that I kicked off the pull request testing in my comment above. My apologies for not noticing sooner!

Jenkins test sanity all jdk8,jdk11,jdk17,jdk21

[hzongaro]

Failure in s390x_linux JDK 17 testing appears to be due to issue #17844.

Failure in x86-64_mac JDK 21 testing appears to be infrastructure-related. Restarting.

Jenkins test sanity xmac jdk21