Prevent commoning of l2a operations in localCSE #7217
Conversation
An l2a operation might be used to convert the result of a compressed reference sequence to an address, but it is also used in other circumstances - for instance, if an Int64 value holds the address of a j9class or a j9method. The l2aEvaluators assume that l2a is used for compressed references or accessing arraylets. If the node is not accessing an arraylet and compressed references are enabled, the evaluator will mark the source register as containing a collected reference under the assumption that it's working with a compressed reference. However, if the l2a is being used for a j9class, j9method, etc., the register that holds it should not be marked as a collected reference. As an interim fix, this change will prevent commoning of l2a operations. The various OMR::<CPU>::MemoryReference::populateMemoryReference code generation methods will skip evaluation of an l2a that has a reference count of one, so that prevents the register containing the operand of the l2a from being marked as a collected reference. In the longer term, OMR issue 6508 proposes introducing a new opcode - l2gcref - that would be used to mark explicitly the result of a conversion from long to an address as holding a collected reference. Once implemented, the change implemented by this commit can be reverted.
|
@vijaysun-omr, may I ask you to review this change? |
| @@ -1186,6 +1186,9 @@ bool OMR::LocalCSE::canBeAvailable(TR::Node *parent, TR::Node *node, TR_BitVecto | |||
| if (node->getOpCodeValue() == TR::allocationFence) | |||
| return false; | |||
|
|
|||
| if (node->getOpCodeValue() == TR::l2a) | |||
There was a problem hiding this comment.
Should TR::i2a also be treated the same way (could be used on 32-bit maybe ?) ?
There was a problem hiding this comment.
If you have time to handle TR::i2a in the next day or so, please do so. Otherwise I won't hold up this fix (assuming all tests pass) past tomorrow, since it is addressing a real failure, and you can add the TR::i2a in a separate PR.
There was a problem hiding this comment.
I hadn't touched the other operations that convert to address, as their evaluators don't seem to mark their registers as collected references. To be safe, I'll capture all of the *2a operators in a follow-on pull request that checks the following in OMR::LocalCSE::canBeAvailable instead:
if (node->getOpCode().isConversion() && node->getOpCode().isRef())
return false;
There was a problem hiding this comment.
Opened pull request #7218 to address this comment.
|
jenkins build all |
|
Tests have passed (except for macOS where the build was cancelled). Merging. |
An
l2aoperation might be used to convert the result of a compressed reference sequence to an address, but it is also used in other circumstances - for instance, if anInt64value holds the address of aj9classor aj9method. Thel2aEvaluators[1,2,3,4] assume thatl2ais used for compressed references or accessing arraylets. If the node is not accessing an arraylet and compressed references are enabled, the evaluator will mark the source register as containing a collected reference under the assumption that it's working with a compressed reference. However, if thel2ais being used for aj9class,j9method, etc., the register that holds it should not be marked as a collected reference.As an interim fix, this change will prevent commoning of
l2aoperations. The variousOMR::<CPU>::MemoryReference::populateMemoryReferencecode generation methods[5,6,7,8] will usually skip evaluation of anl2athat has a reference count of one, so that prevents the register containing the operand of thel2afrom being marked as a collected reference.In the longer term, OMR issue #6508 proposes introducing a new opcode -
l2gcref- that would be used to mark explicitly the result of a conversion from long to an address as holding a collected reference. Once implemented, the change implemented by this commit can be reverted.This will work around the problem reported in OpenJ9 issue eclipse-openj9/openj9#18098 as affecting
SharedClasses.SCM01.MultiThread_1. Other test failures could have other causes that will need further investigation.[1] https://github.com/eclipse/omr/blob/3a4787401fffbdbb8088021d3553915ea3ff8edf/compiler/aarch64/codegen/UnaryEvaluator.cpp#L640-L641
[2] https://github.com/eclipse/omr/blob/3a4787401fffbdbb8088021d3553915ea3ff8edf/compiler/p/codegen/UnaryEvaluator.cpp#L573-L574
[3] https://github.com/eclipse/omr/blob/3a4787401fffbdbb8088021d3553915ea3ff8edf/compiler/x/codegen/UnaryEvaluator.cpp#L381-L382
[4] https://github.com/eclipse/omr/blob/3a4787401fffbdbb8088021d3553915ea3ff8edf/compiler/z/codegen/UnaryEvaluator.cpp#L251-L252
[5] https://github.com/eclipse/omr/blob/3a4787401fffbdbb8088021d3553915ea3ff8edf/compiler/aarch64/codegen/OMRMemoryReference.cpp#L587-L612
[6] https://github.com/eclipse/omr/blob/3a4787401fffbdbb8088021d3553915ea3ff8edf/compiler/p/codegen/OMRMemoryReference.cpp#L587-L597
[7] https://github.com/eclipse/omr/blob/3a4787401fffbdbb8088021d3553915ea3ff8edf/compiler/x/codegen/OMRMemoryReference.cpp#L521-L533
[8] https://github.com/eclipse/omr/blob/3a4787401fffbdbb8088021d3553915ea3ff8edf/compiler/z/codegen/OMRMemoryReference.cpp#L1801-L1809