-
Notifications
You must be signed in to change notification settings - Fork 721
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
Unhide compressed refs logic from Unsafe CAS codegen #20400
base: master
Are you sure you want to change the base?
Conversation
Disables code on Power and Z that hides compressedrefs logic from the fast path code generation for the following recognized methods: sun_misc_Unsafe_compareAndSwapObject_jlObjectJjlObjectjlObject_Z jdk_internal_misc_Unsafe_compareAndExchangeObject jdk_internal_misc_Unsafe_compareAndExchangeReference Fast path code generation for those recognized methods is updated to handle compressedrefs. Original behavior can be restored by setting the TR_UseOldCompareAndSwapObject environment variable. This behavior matches the X implementation which has already disabled the compressedrefs hiding logic when the envvar is not set. Signed-off-by: jimmyk <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of nitpicks. Overall it looks good to me.
// Hiding compressedref logic from CodeGen doesn't seem a good practise, the evaluator always need the uncompressedref node for write barrier, | ||
// therefore, this part is deprecated. It'll be removed once P and Z update their corresponding evaluators. | ||
// Hiding compressedref logic from CodeGen isn't a good practice, and the evaluator still needs the uncompressedref node for write barriers. | ||
// Therefore, this part is deprecated. It can only be activated on X, P or Z with the TR_UseOldCompareAndSwapObject envvar. | ||
static bool UseOldCompareAndSwapObject = (bool)feGetEnv("TR_UseOldCompareAndSwapObject"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something that already in code, but not related to overall changes, Should we change UseOldCompareAndSwapObject
to useOldCompareAndSwapObject
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change this.
static bool UseOldCompareAndSwapObject = (bool)feGetEnv("TR_UseOldCompareAndSwapObject"); | ||
static bool disableCASInlining = feGetEnv("TR_DisableCASInlining") != NULL; | ||
if (self()->comp()->useCompressedPointers() && ((UseOldCompareAndSwapObject && (self()->comp()->target().cpu.isARM64() || !disableCASInlining)) || !(self()->comp()->target().cpu.isX86() || self()->comp()->target().cpu.isARM64()))) | ||
if (((self()->comp()->target().cpu.isX86() && !disableCASInlining) || self()->comp()->target().cpu.isPower() || self()->comp()->target().cpu.isZ()) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, bit condfusing, disableCASInlining
is only checked on x86 not on P and Z. If this is intended, We should have a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intended. The TR_DisableCASInlining
envvar to disable inlining of both Unsafe compareAndSwap
and compareAndExchange
only has support on X. That's why the check is only on X.
Turning on TR_DisableCASInlining
and TR_UseOldCompareAndSwapObject
at the same time doesn't actually make sense since if CAS inlining is disabled, it won't use either of the compare and swap implementations. But, without this change it would crash and I didn't want that.
I will add a comment explaining that the change is because the TR_DisableCASInlining
will take priority and the check is needed to make sure it doesn't crash.
@@ -11952,7 +11952,7 @@ TR::Register* | |||
J9::Z::TreeEvaluator::VMinlineCompareAndSwap(TR::Node *node, TR::CodeGenerator *cg, TR::InstOpCode::Mnemonic casOp, bool isObj, bool isExchange) | |||
{ | |||
TR::Register *scratchReg = NULL; | |||
TR::Register *objReg, *oldVReg, *newVReg; | |||
TR::Register *objReg, *oldVReg, *newVReg, *decompressedValueRegister; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this exists, but we should initialize the registers here with NULL
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change this.
@@ -11969,9 +11969,10 @@ J9::Z::TreeEvaluator::VMinlineCompareAndSwap(TR::Node *node, TR::CodeGenerator * | |||
// compression sequence. | |||
bool isValueCompressedReference = false; | |||
|
|||
TR::Node* decompressedValueNode = newVNode; | |||
static bool UseOldCompareAndSwapObject = (bool)feGetEnv("TR_UseOldCompareAndSwapObject"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the intention was to match the environment set throughout the code base, but we should rename variable name useOldCompareAndSwapObject
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change this.
Added more comments to clarify issues. Renamed UseOldCompareAndSwapObject to useOldCompareAndSwapObject.
My latest commit should address all previous comments. |
Disables code on Power and Z that hides compressedrefs logic from the fast path code generation for the following recognized methods:
sun_misc_Unsafe_compareAndSwapObject_jlObjectJjlObjectjlObject_Z
jdk_internal_misc_Unsafe_compareAndExchangeObject
jdk_internal_misc_Unsafe_compareAndExchangeReference
Fast path code generation for those recognized methods is updated to handle compressedrefs.
Original behavior can be restored by setting the
TR_UseOldCompareAndSwapObject
environment variable. This behavior matches the X implementation which has already disabled the compressedrefs hiding logic when the envvar is not set.