Fix loop strider reassociation and hoisting logic for off-heap - #7509
Conversation
|
For context:
By removing checks for multiply and shift nodes [4], this PR also enables hoisting of dataAddr pointer for arrays with unit stride. [1] UnsafeSetMemory failure console snippet[2] compiler/optimizer/InductionVariable.cpp#L931-L944 "Auto reassociation" |
VermaSh
changed the title
To prevent double hoisting of dataAddr pointer, limit reassociation and hoisting criteria to include only those that have dataAddr pointer as their first child. The offset node doesn't rely on array header size when using dataAddr pointer to get to data elements so no need for reassociation; we can use the original offset node with newly hoisted array load node. Signed-off-by: Shubham Verma <shubhamv.sv@gmail.com>
e747ee8 to
c875d17
Compare
|
|
|
Builds look good, don't see any new failures due to this PR. Marking this ready for review. |
VermaSh
changed the title
|
@vijaysun-omr / @hzongaro can I please get a review |
|
|
||
| TR::SymbolReference *internalPointerSymRef = (*_reassociatedAutos)[originalInternalPointerSymbol]; | ||
| originalNode->getFirstChild()->recursivelyDecReferenceCount(); | ||
| node->decReferenceCount(); |
There was a problem hiding this comment.
What is the rationale for these reference count changes ? This seems to be unrelated to what was discussed in the commit message ?
There was a problem hiding this comment.
From what I understood, refcount of node (offset node) was being decremented because we were replacing it with it's first child. I think that was just carried over from array access trees with array header size because we don't need to recalculate element offset after hoisting dataAddr node in off-heap mode. I removed the decrement refcount logic as part of my fix for 2nd bug.
|
Jenkins build all |
| TR::SymbolReference *internalPointerSymRef = (*_reassociatedAutos)[originalInternalPointerSymbol]; | ||
| originalNode->getFirstChild()->recursivelyDecReferenceCount(); | ||
| node->decReferenceCount(); | ||
| _reassociatedNodes.add(node); |
There was a problem hiding this comment.
Why do we skip adding to _reassociatedNodes ?
There was a problem hiding this comment.
We aren't making any changes to array element offset computation (1n4). We simply hoist the dataAddr pointer node (1n2) and leave the offset node as is, so we don't need to add anything to _reassociatedNodes. Whereas in gencon, we need to update _reassociatedNodes because we hoist array header size (2n5) from offset node (2n3) along with array object load.
off-heap mode:
1n1 aladd (internal pointer)
1n2 contiguousArrayDataAddrFieldSymbol (dataAddrPointer, internal pointer)
1n3 array_obj (pinning array pointer)
1n4 mul/shift/integer offset
non off-heap mode (gencon):
2n1 aladd (internal pointer)
2n2 array_obj (pinning array pointer)
2n3 add/sub offset
2n4 index
2n5 header_size/-header_size
| TR::Node *arrayNode, | ||
| TR::Node *offsetNode = NULL); | ||
| TR::Node *offsetNode = NULL, | ||
| TR::Node *originatingByteCodeNode = NULL); |
There was a problem hiding this comment.
It looks like there are existing calls to generateArrayElementAddressTrees that will get the default originatingByteCodeNode value of NULL, and end up generating TR::Nodes without the correct bytecode info.
For example, there is a call in OMR::TransformUtil::generateFirstArrayElementAddressTrees which is itself called from OMR::ValuePropagation::transformArrayCloneCall. That transformation is applied for calls to Object.clone if the object is known to be an array.
Prior to this change, the IL trees that resulted from that transformation looked like this:
n69n arraycopy <arraycopy>[#215 helper Method] [flags 0x400 0x0 ] (Unsigned cannotOverflow forwardArrayCopy noArrayStoreCheckArrayCopy wordElementArrayCopy ) [0x7fa62c37f550] bci=[-1,18,5] rc=1 vc=0 vn=- li=- udi=- nc=3 flg=0xd020
n66n aladd [0x7fa62c37f460] bci=[-1,17,5] rc=1 vc=0 vn=- li=- udi=- nc=2
n23n ==>aload
n65n lconst 8 (highWordZero X!=0 X>=0 ) [0x7fa62c37f410] bci=[-1,0,3] rc=1 vc=0 vn=- li=- udi=- nc=0 flg=0x4104
n68n aladd [0x7fa62c37f500] bci=[-1,18,5] rc=1 vc=0 vn=- li=- udi=- nc=2
n25n ==>newarray
n67n lconst 8 (highWordZero X!=0 X>=0 ) [0x7fa62c37f4b0] bci=[-1,0,3] rc=1 vc=0 vn=- li=- udi=- nc=0 flg=0x4104
n64n lmul [0x7fa62c37f3c0] bci=[-1,18,5] rc=1 vc=0 vn=- li=- udi=- nc=2
n62n i2l [0x7fa62c37f320] bci=[-1,18,5] rc=1 vc=0 vn=- li=- udi=- nc=1
n55n ==>arraylength
n63n lconst 4 (highWordZero X!=0 X>=0 ) [0x7fa62c37f370] bci=[-1,18,5] rc=1 vc=0 vn=- li=- udi=- nc=0 flg=0x4104
after this change, they look like this:
n71n treetop [0x7f52fa57f5f0] bci=[-1,18,5] rc=0 vc=0 vn=- li=- udi=- nc=1
n69n arraycopy <arraycopy>[#215 helper Method] [flags 0x400 0x0 ] (Unsigned cannotOverflow forwardArrayCopy noArrayStoreCheckArrayCopy wordElementArrayCopy ) [0x7f52fa57f550] bci=[-1,18,5] rc=1 vc=0 vn=- li=- udi=- nc=3 flg=0xd020
n66n aladd [0x7f52fa57f460] bci=[-1,0,3] rc=1 vc=0 vn=- li=- udi=- nc=2
n23n ==>aload
n65n lconst 8 (highWordZero X!=0 X>=0 ) [0x7f52fa57f410] bci=[-1,0,3] rc=1 vc=0 vn=- li=- udi=- nc=0 flg=0x4104
n68n aladd [0x7f52fa57f500] bci=[-1,0,3] rc=1 vc=0 vn=- li=- udi=- nc=2
n25n ==>newarray
n67n lconst 8 (highWordZero X!=0 X>=0 ) [0x7f52fa57f4b0] bci=[-1,0,3] rc=1 vc=0 vn=- li=- udi=- nc=0 flg=0x4104
n64n lmul [0x7f52fa57f3c0] bci=[-1,18,5] rc=1 vc=0 vn=- li=- udi=- nc=2
n62n i2l [0x7f52fa57f320] bci=[-1,18,5] rc=1 vc=0 vn=- li=- udi=- nc=1
n55n ==>arraylength
n63n lconst 4 (highWordZero X!=0 X>=0 ) [0x7f52fa57f370] bci=[-1,18,5] rc=1 vc=0 vn=- li=- udi=- nc=0 flg=0x4104
In particular, notice the bytecode info. on the aladd nodes.
3 participants
To prevent double hoisting of dataAddr pointer, limit reassociation and hoisting criteria to include only those that have dataAddr pointer as their first child.
The offset node doesn't rely on array header size when using dataAddr pointer to get to data elements so no need for reassociation; we can use the original offset node with newly hoisted array load node.