-
Notifications
You must be signed in to change notification settings - Fork 720
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
Implement inlined version of sun_misc_Unsafe_copyMemory #7136
Comments
FYI @fjeremic |
I question why this belongs to the codegen in the first place? This seems like something that should be reduced to a Looking into this deeper on x86 [1] and Power [2] it looks like we have duplicate code here in the codegen evaluators. It seems however there already is a common optimization which transforms these calls into Rather than reimplementing this on every codegen we should examine whether the current x86 and Power implementations in the codegen are needed or if the VP pass is enough to capture all the use cases. If it is not, we should likely focus on improving the VP pass, or alternatively moving this logic to J9RecognizedCallTransformer since it's a simple method to tree transformation that runs at IL gen time. We can then deprecate the x86 and Power codegen implementations and benefit from this transformation on all codegens for free. |
Of course, if the semantics of the primitive copy are identical then a cross-platform approach is always welcome. |
@andrewcraik We compared the VP code and the cases in X86 and Power. I believe VP code has already covered what X86 and Power are doing. And we are now thinking of:
|
@andrewcraik Could you please help take a look at the actions mentioned above and see if they are doable? |
So doing the transform in the recognized call transformer is likely doable. A general opt in VP to set the forward/backward flag is much more dangerous - arraycopy nodes come from a lot of places so while we should be able to set the flags in VP based on constraints there is a real risk of bugs being exposed and having to back off because of specialized use of the arraycopy (main ones in this category of fear in my mind are the ones from IdiomRecognition). Care will need to be taken to check the details of the different codegens in how their arraycopy works and the flags they check and the impact they have. While this should be consistent there is a real risk of inconsistency leading to potential bugs. Simply implementing the above and passing the tests is not going to be enough analysis will need to be done to try to ensure the transformations won't leave bugs lurking since this is a tricky area. |
@andrewcraik We tried to add an assertion fail in https://github.com/eclipse/omr/blob/ffb0d9866e44551d22553be84adada6e546f0a19/compiler/optimizer/ValuePropagationCommon.cpp#L1024-L1025 |
I don't think the fact it isn't seen in a personal build is a reason to drop the support of setting the direction flag. If the constraint handling for arraycopy can handle this then we should be good. If not that enhancement is needed. |
The code in VP would be dead if we migrate the transformation to J9RecognizedCallTransformer as VP will never see a call to a recognized method IMO if the constraint piece of code setting the arraycopy flag is not hit in all of our testing, how can we even be sure the constraint piece of code in VP works in the first place? |
I believe this pr satisfies the purpose of this issue. In the mentioned pr, the transformation was moved from arch specific codeGen to
in Post Lower Trees:
|
x86 and Power provide inlined implementations of
sun_misc_Unsafe_copyMemory
. Investigate the semantics of this method, its current relevance, and implement on Z, AArch64, and AArch32 as appropriate.The text was updated successfully, but these errors were encountered: