-
Notifications
You must be signed in to change notification settings - Fork 398
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
Value Types: Replace arraycopy source and destination with temps #7582
base: master
Are you sure you want to change the base?
Value Types: Replace arraycopy source and destination with temps #7582
Conversation
When preparing for null restricted arraycopy transformation, there are temps created for the source array and the destination array. Other sub transformations that run after the preparation and before null-restrcited arraycopy transformation are not aware of these temps. These temps might not get updated properly. This change replaces all the commoned source array node and commoned destination array node with the temps right after the temps are created. Fixes: eclipse-openj9/openj9#20522 Signed-off-by: Annabelle Huo <[email protected]>
@hzongaro May I ask you to review this change? Thank you! |
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 think these changes look good. Just a few suggestions.
May I also ask you to fix a couple of typos in the commit comment? "null-restrcited" should be "null-restricted". Also, it's written without a hyphen in one spot, and with a hyphen in another spot.
@@ -922,6 +922,47 @@ bool OMR::ValuePropagation::transformUnsafeCopyMemoryCall(TR::Node *arrayCopyNod | |||
return false; | |||
} | |||
|
|||
static void setNodeOnList(TR::Node *currNode, TR_BitVector *nodeList) |
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.
May I ask you to add some Doxygen comments or at least some description of what it's doing? I know there's a bit of a description that appears where it's called, but I think it would help to have it described here.
} | ||
} | ||
|
||
static void findAndReplaceCommonedNodeWithLoadFromTemp(TR::Node *currNode, TR::Node *matchingNode, TR::SymbolReference *symRef, TR_BitVector *nodeSkipList, bool trace, TR::Compilation *comp) |
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.
May I ask you to add some Doxygen comments or at least some description of what it's doing? I know there's a bit of a description that appears where it's called, but I think it would help to have it described here.
if (trace) | ||
traceMsg(comp, "%s: currNode n%dn replace childNode n%dn with loadNode n%dn\n", __FUNCTION__, currNode->getGlobalIndex(), childNode->getGlobalIndex(), loadNode->getGlobalIndex()); | ||
} | ||
else if (!nodeSkipList->isSet(childNode->getGlobalIndex())) |
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 find it a little bit confusing that a node that is in nodeSkipList
might get processed by the if
that appears just before this else if
if its parent was not in nodeSkipList
. I wonder if it should be called skipDescendantsList
or something like that. Just a thought.
findAndReplaceCommonedNodeWithLoadFromTemp(itNode, dstObjNode, dstArrRefSymRef, nodeSkipList, trace(), comp()); | ||
findAndReplaceCommonedNodeWithLoadFromTemp(itNode, srcObjNode, srcArrRefSymRef, nodeSkipList, trace(), comp()); |
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 that the problem this is fixing appeared with temporaries for the destination or source arrays, but is it conceivable that the same problem could occur with the offset or length values?
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.
Good question! I think theoretically speaking the same problem could occur with the offset and length as well. I tried to tweak my unit tests to show this problem on the offset or length, but so far I haven't been able to get the expected IL trees. I'll think over the proper fix.
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'm able to reproduce the same problem with copy size now. Below example of IL trees shows the copy size is stored to a temp #474
after the preparation of null restricted array copy transformation. If the slow path block_23
is invoked, #474
is uninitialized.
I will expand this change to all children of arraycopy in the next commit.
...
n180n istore <temp slot 12>[#465 Auto] [flags 0x3 0x0 ] [0x7f63f9f82800] bci=[-1,37,129] rc=0 vc=0 vn=- li=- udi=- nc=1
n32n iload <auto slot 3>[#421 Auto] [flags 0x3 0x0 ] (cannotOverflow ) [0x7f63f9ee5120] bci=[-1,36,129] rc=2 vc=51 vn=- li=- udi=- nc=0 flg=0x1000
n368n istore <temp slot 24>[#477 Auto] [flags 0x3 0x0 ] [0x7f63f9f862c0] bci=[-1,36,129] rc=0 vc=0 vn=- li=- udi=- nc=1
n32n ==>iload
n365n ificmpne --> block_23 BBStart at n168n () [0x7f63f9f861d0] bci=[-1,0,125] rc=0 vc=0 vn=- li=- udi=- nc=2 flg=0x20
n363n iand [0x7f63f9f86130] bci=[-1,0,125] rc=1 vc=0 vn=- li=- udi=- nc=2
n361n iloadi <isClassFlags>[#340 Shadow +36] [flags 0x603 0x0 ] [0x7f63f9f86090] bci=[-1,0,125] rc=1 vc=0 vn=- li=- udi=- nc=1
n360n aloadi <vft-symbol>[#341 Shadow] [flags 0x18607 0x0 ] [0x7f63f9f86040] bci=[-1,0,125] rc=1 vc=0 vn=- li=- udi=- nc=1
n357n aload <temp slot 10>[#463 Auto] [flags 0x7 0x0 ] [0x7f63f9f85f50] bci=[-1,0,125] rc=1 vc=0 vn=- li=- udi=- nc=0
n362n iconst 0x2000000 (X!=0 X>=0 ) [0x7f63f9f860e0] bci=[-1,0,125] rc=1 vc=0 vn=- li=- udi=- nc=0 flg=0x104
n364n iconst 0 (X==0 X>=0 X<=0 ) [0x7f63f9f86180] bci=[-1,0,125] rc=1 vc=0 vn=- li=- udi=- nc=0 flg=0x302
n405n BBEnd </block_14> ===== [0x7f63f9f86e50] bci=[-1,0,125] rc=0 vc=0 vn=- li=- udi=- nc=0
...
n366n BBStart <block_24> (freq 9500) [0x7f63f9f86220] bci=[-1,36,129] rc=0 vc=0 vn=- li=- udi=- nc=0
n292n istore <temp slot 21>[#474 Auto] [flags 0x3 0x0 ] [0x7f63f9f84b00] bci=[-1,36,129] rc=0 vc=0 vn=- li=- udi=- nc=1
n372n iload <temp slot 24>[#477 Auto] [flags 0x3 0x0 ] [0x7f63f9f86400] bci=[-1,36,129] rc=1 vc=0 vn=- li=- udi=- nc=0
...
n289n BBStart <block_17> (freq 9500) [0x7f63f9f84a10] bci=[-1,37,129] rc=0 vc=0 vn=- li=- udi=- nc=0
n36n ireturn [0x7f63f9ee5260] bci=[-1,41,130] rc=0 vc=51 vn=- li=- udi=- nc=1
n298n iload <temp slot 21>[#474 Auto] [flags 0x3 0x0 ] [0x7f63f9f84ce0] bci=[-1,36,129] rc=1 vc=0 vn=- li=- udi=- nc=0
n2n BBEnd </block_17> ===== [0x7f63f9ee47c0] bci=[-1,41,130] rc=0 vc=50 vn=- li=- udi=- nc=0
When preparing for null-restricted arraycopy transformation, there are temps created for the source array and the destination array. Other sub transformations that run after the preparation and before null-restricted arraycopy transformation are not aware of these temps. These temps might not get updated properly. This change replaces all the commoned source array node and commoned destination array node with the temps right after the temps are created.
Fixes: eclipse-openj9/openj9#20522