Enable arrayset for Unsafe.setMemory0#8291
Conversation
|
@Deigue Do you plan to keep the commit history? Or will you squash these into a single commit? Looking at your commits and the code change itself, I would recommend squashing into a single commit. |
|
Definitely planning to squash into one, was just seeing if there is more feedback/changes to add. Otherwise I will squash now itself. |
|
Let's squash and write a clean commit message, and then we can start reviewing the changes in their final state. |
|
|
||
| switch (_destType) { | ||
| case TR::Int8: | ||
| cursor = generateRXInstruction(_cg, TR::InstOpCode::STC, _dstNode, _initReg, newMR); |
There was a problem hiding this comment.
If you wanted to separate this part of the change into a separate commit, I would be okay with that since it's logically different than the rest of the changes (which are moreso about enabling the transformation). The PR title and first message in the PR should also probably mention that we are also adding support for bytes.
Deigue
force-pushed
the
1003-unsafesetmemory
branch
from
184ec12 to
145f6c9
Compare
| TR_S390ScratchRegisterManager *generateScratchRegisterManager(int32_t capacity = 8); | ||
|
|
||
| bool canTransformUnsafeCopyToArrayCopy(); | ||
| bool canTransformUnsafeSetMemory(); |
There was a problem hiding this comment.
Can we add doxygen comments for this new API? See lines 238-257 for examples.
|
|
||
| switch (_destType) { | ||
| case TR::Int8: | ||
| cursor = generateRXInstruction(_cg, TR::InstOpCode::STC, _dstNode, _initReg, newMR); |
There was a problem hiding this comment.
This new addition looks reasonable to me. It would be good to test this OMR change in a personal build by itself (without your own openj9 changes) just to make sure the commit is clean.
Deigue
force-pushed
the
1003-unsafesetmemory
branch
from
145f6c9 to
40d8e72
Compare
Store instruction handling bytes is missing and would fallback to STG. This adds the appropriate case to pick the appropriate instruction. Signed-off-by: Gaurav Chaudhari <Gaurav.Chaudhari@ibm.com>
Signed-off-by: Gaurav Chaudhari <Gaurav.Chaudhari@ibm.com>
Deigue
force-pushed
the
1003-unsafesetmemory
branch
from
40d8e72 to
c2ae9e0
Compare
|
@r30shah Can I request a review from you as well for this PR? |
r30shah
left a comment
r30shah
left a comment
There was a problem hiding this comment.
OpMemToMem.cpp changes looks good to me. I do not think other changes should in the OMR (It should be moved to OpenJ9).
| { "enableTraps", "C\tenable trap instructions", RESET_OPTION_BIT(TR_DisableTraps), "F" }, | ||
| { "enableTreePatternMatching", "O\tEnable opts that use the TR_Pattern framework", | ||
| RESET_OPTION_BIT(TR_DisableTreePatternMatching), "F" }, | ||
| { "enableUnsafeSetMemoryArrayset", "O\tenable arrayset for Unsafe.setMemory0", |
There was a problem hiding this comment.
Can we rename this option to something more relatable ? The option should give an indication that it is enabling accelerating Unsafe.setMemory0 - not necessarily tied to the node it is lowered to.
Also - There is no notion of unsafe in OMR - either move this option to OpenJ9 or guard it with J9 project specific macro.
There was a problem hiding this comment.
Keep in mind - if the intention to have this option so that it can be applied per method, it needs to be in OMR.
There was a problem hiding this comment.
So the equivalent would be like the ones in J9Options.cpp/hpp instead? I guess it is contingent on the other comment, whether I can keep it in OMRCodeGenerator to match the POWER equivalent behavior, or change things and move it to J9. Because then, I would need the TR option accessible in OMR . if I am keeping the canTransform... bool here.
There was a problem hiding this comment.
on that note, would enableAccelerateUnsafeSetMemory be acceptable?
Or accelerateUnsafeSetMemory if the prefix is not a must?
There was a problem hiding this comment.
I was thinking to control enablement through environment flag. This is the kind of acceleration that we want to happen by default so rather than using entry in the Options, I would use feGetEnv in the code where you are transforming to enable. We do not need to use entry in options for this one.
| return (!disableTran && !comp()->getCurrentMethod()->isJNINative() && !comp()->getOption(TR_DisableArrayCopyOpts)); | ||
| } | ||
|
|
||
| bool OMR::Z::CodeGenerator::canTransformUnsafeSetMemory() |
There was a problem hiding this comment.
Similar to previous comment - This should be in the J9CodeGenerator as it is accelerating Unsafe API in Java. We should not have this in OMR.
There was a problem hiding this comment.
We have this in the power equivalent already in OMR:
omr/compiler/p/codegen/OMRCodeGenerator.hpp
Line 220 in d4356d4
Inheriting from:
omr/compiler/compile/OMRCompilation.hpp
Line 1108 in d4356d4
I just re-defined on the Z level
There was a problem hiding this comment.
I understand, but I do not see any reason that even on P - it should be in OMR. Even the user of the flag is in OpenJ9. I would simply propose to move this to J9 and clean-up OMR.
There was a problem hiding this comment.
Add OpenJ9 equivalents: eclipse-openj9/openj9#24229
Remove OMR bools: #8301
3 participants
Associated with: https://github.com/eclipse-openj9/openj9/pull/23854/changes
Adds JIT TR option to enable arrayset for Unsafe.setMemory0
and adds missing Int8/byte case to pick the correct STC instruction for OpMemToMem (used by the arraysetEvaluator)