Add support for inlining new atomic methods on Power #3018
Conversation
Added support for inlining of the new atomic methods
atomic{AddAndFetch|FetchAndAdd|Swap}{32|64}BitSymbol on Power. This is
largely a copy of similar support in OpenJ9 for inlining methods of
AtomicInteger and AtomicLong.
Signed-off-by: Henry Zongaro <[email protected]>
|
May I ask @0dvictor @zl-wang @gita-omr to review? One thing I'll point out is that I put There will be a separate PR in OpenJ9 to take advantage of this change. |
|
|
||
| return doInline; | ||
| } | ||
|
|
There was a problem hiding this comment.
High-level issue: this will not work for 32bit JVM. i.e. this is assuming a 64bit JVM.
There was a problem hiding this comment.
@zl-wang Julian, thanks for catching this. Just so I'm clear, can this code be inlined on a 32 bit JVM when it's working with 32 bit operands, and inlined on a 64 bit JVM when it's working with either 32 bit and 64 bit operands? Or can it only be inlined on a 64 JVM ever?
Something I just noticed this morning, I was playing with AtomicLong.getAndAdd and AtomicInteger.getAndAdd without my changes for these new intrinsics, and it looks like inline code is never generated for AtomicLong.getAndAdd for 32 bit or 64 bit JVMs, and always generated for AtomicInteger.getAndAdd for both 32 bit and 64 bit JVMs. Is that expected?
There was a problem hiding this comment.
@hzongaro I didn't expect that. I expected existing JVM can inline CAS of both 32/64 operands in both 32/64 bit JVM. The only assumption is the underlying CPU is 64bit (this has been true since java7.1 when we discontinued support of 32bit hardware). The case for inlining CAS of 64bit operand in 32bit JVM needs special care though ... you can assemble 64bit operand in a register within the CAS range only, depending on the fact that CAS will definitely fail if there was a signal delivered in the between (i.e. meaning the upper 32bit of the register can be trashed anytime).
There was a problem hiding this comment.
@zl-wang Julian, I've been working on a revision that handles 64-bit values running in a 32-bit JVM, but it makes the code quite ugly. I’ll pull that out as a separate pull request, and I’ll let you decide whether it’s an important enough case to handle, as well as point out any problems with the way that I tackled the problem.
Added support for inlining of the new atomic methods
atomic{AddAndFetch|FetchAndAdd|Swap}{32|64}BitSymbol on Power. This is
largely a copy of similar support in OpenJ9 for inlining methods of
AtomicInteger and AtomicLong.
Signed-off-by: Henry Zongaro <[email protected]>
…to atomic-power-alt
Pull Request 2999 changed replaced use of supportsAtomicAdd with supportsNonHelper in order to report whether various atomic operations can be inlined. Revised Power codegen to use the same. Signed-off-by: Henry Zongaro <[email protected]>
Address various review comments, including:
- Look for opportunity to use add immediate instructions for 64-bit
constant values that are in range.
- Rename resultReg to oldValueReg to better reflect its use
- Avoid an unnecessary copy of the delta value into a new register every
time through the l{w|d}arx/st{w|d}arx loop.
- Added assertion to verify the number of register dependencies.
Updated inlineDirectCall to reflect new names of nonhelper methods.
Signed-off-by: Henry Zongaro <[email protected]
Removed a bad line break from OMRCodeGenerator.hpp and added a const declaration for a variable that's never modified. Signed-off-by: Henry Zongaro <[email protected]>
|
@hzongaro I think you accidentally checked in some huge file as part of your latest commit. |
Removed 'code' file that I had accidentally created. Signed-off-by: Henry Zongaro <[email protected]>
Removed calls to stopUsingRegister that were incorrect due to calls to TR::Node::decReferenceCount that would affect the same register. Also fixed an incorrect check for whether an 8 byte integer value is in the range of a 4 byte value that relied on the machine's endianness. Signed-off-by: Henry Zongaro <[email protected]>
|
I believe all review comments have been addressed, so I will remove "WIP" from the title. |
hzongaro
changed the title
|
@genie-omr build plinux,aix |
Added support for inlining of the new atomic intrinsics
atomic{AddAndFetch|FetchAndAdd|Swap}{32|64}BitSymbol on Power. This is
largely a copy of similar support in OpenJ9 for inlining methods of
AtomicInteger and AtomicLong on Power.
Issue: #2958
Signed-off-by: Henry Zongaro [email protected]