Commoning and InductionVariable changes for OffHeap #7368
Conversation
Signed-off-by: Shubham Verma <shubhamv.sv@gmail.com>
The dataAddrPointer holds an internal-pointer and commoning it requires handling cases of pinning-array movement. This commit disables commoning at least for now. be77a52 has the code to have pinning-array slot in dataAddrPointer but needs further testing with commoning enabled as the internal-pointer nodes previously had assumed opcodes and tree shapes. Signed-off-by: Abdulrahman Alattas <rmnattas@gmail.com>
rmnattas
force-pushed
the
off-heap-merge3
branch
from
7f6ce78
to
edb1bc6
Compare
|
Fixing wrapping some code by a macro, but ready for review. |
rmnattas
force-pushed
the
off-heap-merge3
branch
2 times, most recently
from
ecd11ed
to
512b918
Compare
compiler/il/OMRNode.cpp
Outdated
| @@ -3807,7 +3821,7 @@ OMR::Node::createStoresForVar(TR::SymbolReference * &nodeRef, TR::TreeTop *inser | |||
| } | |||
|
|
|||
| if (isInternalPointer && | |||
| self()->getOpCode().isArrayRef() && | |||
| (self()->getOpCode().isArrayRef() || self()->isDataAddrPointer())&& | |||
There was a problem hiding this comment.
Do we need these changes to check if self is a data address pointer (this line and the following lines changed in this same routine) ?
I ask, since it seems like we return early from this routine if self is a data address pointer anyway.
There was a problem hiding this comment.
Correct, removing redundant checks
| @@ -3643,6 +3651,79 @@ bool TR_LoopStrider::reassociateAndHoistComputations(TR::Block *loopInvariantBlo | |||
| } | |||
| } | |||
|
|
|||
| #ifdef J9_PROJECT_SPECIFIC | |||
There was a problem hiding this comment.
I don't think this is part of just a simple disabling of the logic for data address pointer (?) If so, are you able to add some code comment descriptions for this code and others like it in this commit to explain what is being done ? Note that the commit message may also need to be updated in such case.
There was a problem hiding this comment.
The code changed without updating the commit.
The final code teaches inductionVariable about OffHeap array access tree structure and skipping examining dataAddress nodes as traditional internalPointers (trying to find the induction variable in its children).
Induction variable can hoist computations for OffHeap.
rmnattas
force-pushed
the
off-heap-merge3
branch
from
512b918
to
ed26d4f
Compare
The final code teaches inductionVariable about OffHeap array access tree structure and skipping examining dataAddress nodes as traditional internalPointers (trying to find the induction variable in its children). Induction variable can hoist computations for OffHeap. Signed-off-by: Abdulrahman Alattas <rmnattas@gmail.com>
rmnattas
force-pushed
the
off-heap-merge3
branch
from
ed26d4f
to
8fabd3a
Compare
rmnattas
changed the title
| if (pair->_indexSymRef == pinningArrayNode->getSymbolReference()) | ||
| { | ||
| pinningArrayPointer = pair->_derivedSymRef->getSymbol()->castToAutoSymbol(); | ||
| internalPointerSymbol = pair->_derivedSymRef->getReferenceNumber(); |
There was a problem hiding this comment.
I am not sure this statement (or indeed the loop) is valid to run for a data address pointer case (?) I guess I am wondering if we will create an internal pointer for the aladd/aiadd used for the array access, OR for the data address pointer itself, that is just a part of the aladd/aiadd IL tree, and how we would know which case the internal pointer is for.
There was a problem hiding this comment.
I think I understand your question, in offheap we won't hoist an internal pointer so this might not need to be changed for dataAddr. We hoist two values to the loop pre-header (both not internal pointers), the baseArray and the offset. What's left done in the array is loading the dataAddr of the baseArray and adding the offset to it, and incrementing the offset.
Maybe that code is never reached for offheap... not sure. But we're changing the use of node->getFirstChild() to pinningArrayNode for all cases.
for (int i = 0; i < 64; i++) {
sum += inst.x[i+32];
}
loop pre-header
n444n astore <temp slot 7>
n32n aloadi Loop.x [I[ // baseArray
n613n lstore <temp slot 11> // offset
n612n ladd [ 0x3fcaeb8af00] bci=[-1,27,24] rc=1 vc=0 vn=- li=-1 udi=- nc=2
n610n lmul [ 0x3fcaeb8ae60] bci=[-1,27,24] rc=1 vc=0 vn=- li=-1 udi=- nc=2
n608n lload <temp slot 10> // i
n609n lconst 4 (highWordZero X!=0 X>=0 ) [ 0x3fcaeb8ae10] bci=[-1,27,24] rc=1 vc=701 vn=- li=-1 udi=- nc=0 flg=0x4104
n611n lconst 128 (highWordZero X!=0 X>=0 ) [ 0x3fcaeb8aeb0] bci=[-1,27,24] rc=1 vc=0 vn=- li=-1 udi=- nc=0 flg=0x4104
n52n BBStart <block_6> (freq 10000) (in loop 6) [ 0x3fcaeb80000] bci=[-1,29,24] rc=0 vc=703 vn=- li=- udi=- nc=0
n75n istore <auto slot 2>[#406 Auto] [flags 0x3 0x0 ] [ 0x3fcaeb80730] bci=[-1,48,25] rc=0 vc=703 vn=- li=8 udi=5 nc=1
n74n iadd [ 0x3fcaeb806e0] bci=[-1,47,25] rc=1 vc=703 vn=- li=- udi=- nc=2
n73n iloadi <array-shadow>[#254 Shadow] [flags 0x80000603 0x0 ] (cannotOverflow ) [ 0x3fcaeb80690] bci=[-1,46,25] rc=1 vc=703 vn=- li=- udi=- nc=1 flg=0x1000
n72n aladd (X>=0 internalPtr ) [ 0x3fcaeb80640] bci=[-1,46,25] rc=1 vc=703 vn=- li=- udi=- nc=2 flg=0x8100
n68n aloadi <contiguousArrayDataAddrFieldSymbol>[#355 final Shadow +8] [flags 0x20604 0x0 ] (internalPtr ) [ 0x3fcaeb80500] bci=[-1,46,25] rc=1 vc=703 vn=- li=- udi=- nc=1 flg=0x8000
n57n aload <temp slot 7>[#432 Auto] [flags 0x7 0x0 ] (X!=0 X>=0 ) [ 0x3fcaeb80190] bci=[-1,38,25] rc=1 vc=703 vn=- li=- udi=12 nc=0 flg=0x104
n71n lload <temp slot 11>[#436 Auto] [flags 0x4 0x0 ] (highWordZero X>=0 cannotOverflow ) [ 0x3fcaeb805f0] bci=[-1,46,25] rc=2 vc=703 vn=- li=- udi=- nc=0 flg=0x5100
n55n iload <auto slot 2>[#406 Auto] [flags 0x3 0x0 ] (cannotOverflow ) [ 0x3fcaeb800f0] bci=[-1,36,25] rc=1 vc=703 vn=- li=- udi=14 nc=0 flg=0x1000
// increment i
n79n lstore <temp slot 10>[#435 Auto] [flags 0x4 0x0 ] [ 0x3fcaeb80870] bci=[-1,49,24] rc=0 vc=703 vn=- li=9 udi=6 nc=1
n601n lsub (X>=0 cannotOverflow ) [ 0x3fcaeb8ab90] bci=[-1,49,24] rc=2 vc=703 vn=- li=- udi=- nc=2 flg=0x1100
n587n lload <temp slot 10>[#435 Auto] [flags 0x4 0x0 ] [ 0x3fcaeb8a730] bci=[-1,41,25] rc=1 vc=703 vn=- li=- udi=- nc=0
n600n lconst -1 (X!=0 X<=0 ) [ 0x3fcaeb8ab40] bci=[-1,49,24] rc=1 vc=703 vn=- li=- udi=- nc=0 flg=0x204
// increment offset
n618n lstore <temp slot 11>[#436 Auto] [flags 0x4 0x0 ] [ 0x3fcaeb8b0e0] bci=[-1,46,25] rc=0 vc=0 vn=- li=-1 udi=- nc=1
n617n ladd [ 0x3fcaeb8b090] bci=[-1,46,25] rc=1 vc=0 vn=- li=-1 udi=- nc=2
n71n ==>lload
n616n lmul [ 0x3fcaeb8b040] bci=[-1,49,24] rc=1 vc=0 vn=- li=-1 udi=- nc=2
n615n lconst 1 (highWordZero X!=0 X>=0 ) [ 0x3fcaeb8aff0] bci=[-1,49,24] rc=1 vc=699 vn=- li=-1 udi=- nc=0 flg=0x4104
n614n lconst 4 (highWordZero X!=0 X>=0 ) [ 0x3fcaeb8afa0] bci=[-1,27,24] rc=1 vc=701 vn=- li=-1 udi=- nc=0 flg=0x4104
|
Jenkins build all |
|
Jenkins build win |
Code disables localCSE for offheap and updates
createStoreForVarto only store the baseArray (not the dataAddressPointer). Code to store pinningArray for offheap is in #7345 but not tested with commoning enabled yet.Also changes to teach InductionVariable code on OffHeap tree structure.