Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update BCDCHK uncommoning for off-heap #20302

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update BCDCHK uncommoning for off-heap #20302

wants to merge 1 commit into from
+34 −6

Conversation

VermaSh
Copy link
Contributor

@VermaSh VermaSh commented Oct 4, 2024

Update BCDCHK uncommoning for off-heap because address node of BCDCHK can be aloadi with single child if off-heap is enabled.

@VermaSh
Copy link
Contributor Author

VermaSh commented Oct 4, 2024

cc: @r30shah @zl-wang @rmnattas

@VermaSh VermaSh marked this pull request as draft October 4, 2024 15:54
@VermaSh
Copy link
Contributor Author

VermaSh commented Oct 4, 2024 •
edited
Loading

Personal build without off-heap changes to verify that I am not missing any dependencies. Internal issue tracking the failure.

Context into the failure:
The address node (contiguousArrayDataAddrFieldSymbol node) in BCDCHK was being uncommoned incorrectly. The contiguousArrayDataAddrFieldSymbol node was being recreated as a normal aloadi node, from the uncommoning logic it was expecting the aloadi to be aladd and have 2 children [2]. The test is passing on my local machine, will wait for my personal build [3] to pass before opening a PR for the patch [1].

n2324n    treetop                                                                             [     0x3fcd425a620] bci=[3,42,580] rc=0 vc=585 vn=- li=28 udi=- nc=1
n2315n      aloadi  <contiguousArrayDataAddrFieldSymbol>[#355  final Shadow +8] [flags 0x20604 0x0 ] (internalPtr )  [     0x3fcd425a350] bci=[3,42,580] rc=2 vc=585 vn=- li=28 udi=
n2437n        ==>aRegLoad
n2322n    BCDCHK [#847] ()                                                                    [     0x3fcd425a580] bci=[3,30,576] rc=0 vc=585 vn=- li=28 udi=- nc=7 flg=0x20
n2321n      pdshlOverflow <prec=5 (len=3) adj=0 round=0> sign=?                               [     0x3fcd425a530] bci=[-1,30,2376] rc=2 vc=585 vn=- li=-1 udi=- nc=2
n2313n        l2pd <prec=19 (len=10) srcprec=4> sign=?  (X>=0 )                               [     0x3fcd425a2b0] bci=[-1,30,2376] rc=1 vc=585 vn=- li=- udi=- nc=1 flg=0x100
n1926n          lconst 1230 (highWordZero X!=0 X>=0 )                                         [     0x3fcd42529c0] bci=[-1,30,2376] rc=2 vc=585 vn=- li=28 udi=- nc=0 flg=0x4104
n32n          ==>iconst 0
n2315n      ==>aloadi
n1926n      ==>lconst 1230
n2437n      ==>aRegLoad
n32n        ==>iconst 0
n1928n      iconst 5 (X!=0 X>=0 )                                                             [     0x3fcd4252a60] bci=[-1,35,2376] rc=3 vc=585 vn=- li=28 udi=- nc=0 flg=0x104
n32n        ==>iconst 0
...
Performing UncommonBCDCHKAddressNode
O^O UNCOMMON BCDCHK ADDRESS NODE: Replacing node aloadi [000003FCD425A350] with [000003FCD425DF00]
O^O UNCOMMON BCDCHK ADDRESS NODE: Replacing node aloadi [000003FCD4259FE0] with [000003FCD425DF50]
...
n2324n    treetop                                                                             [     0x3fcd425a620] bci=[3,42,580] rc=0 vc=587 vn=- li=28 udi=- nc=1
n2315n      aloadi  <contiguousArrayDataAddrFieldSymbol>[#355  final Shadow +8] [flags 0x20604 0x0 ] (internalPtr )  [     0x3fcd425a350] bci=[3,42,580] rc=1 vc=587 vn=- li=28 udi=
n2437n        ==>aRegLoad
n2322n    BCDCHK [#847] ()                                                                    [     0x3fcd425a580] bci=[3,30,576] rc=0 vc=587 vn=- li=28 udi=- nc=7 flg=0x20
n2321n      pdshlOverflow <prec=5 (len=3) adj=0 round=0> sign=?                               [     0x3fcd425a530] bci=[-1,30,2376] rc=2 vc=587 vn=- li=-1 udi=- nc=2
n2313n        l2pd <prec=19 (len=10) srcprec=4> sign=?  (X>=0 )                               [     0x3fcd425a2b0] bci=[-1,30,2376] rc=1 vc=587 vn=- li=- udi=- nc=1 flg=0x100
n1926n          lconst 1230 (highWordZero X!=0 X>=0 )                                         [     0x3fcd42529c0] bci=[-1,30,2376] rc=2 vc=587 vn=- li=28 udi=- nc=0 flg=0x4104
n32n          ==>iconst 0
n2506n      aloadi                                                                            [     0x3fcd425df00] bci=[3,30,576] rc=1 vc=0 vn=- li=- udi=- nc=2
n2437n        ==>aRegLoad

[1] compare/off-heap...VermaSh:openj9:off-heap_daa_failure?expand=1 "Fix patch"
[2] runtime/compiler/z/codegen/UncommonBCDCHKAddressNode.cpp#L69-L75 "Expecting the address node to be aladd with 2 children"
[3] off-heap personal build

@VermaSh
Update BCDCHK uncommoning for off-heap
c934487 
Update BCDCHK uncommoning for off-heap because address node of BCDCHK
can be aloadi with single child if off-heap is enabled.

Signed-off-by: Shubham Verma <[email protected]>
@VermaSh
Copy link
Contributor Author

VermaSh commented Oct 7, 2024 •
edited
Loading

Relaunched personal build without off-heap changes because previous had a lot of infra related failures.

@VermaSh
Copy link
Contributor Author

VermaSh commented Oct 10, 2024

Didn't see any failures related to this PR. @r30shah / @zl-wang this is ready for review.

@VermaSh VermaSh marked this pull request as ready for review October 10, 2024 19:39
@VermaSh VermaSh changed the title WIP: Update BCDCHK uncommoning for off-heap Update BCDCHK uncommoning for off-heap Oct 10, 2024
r30shah
r30shah requested changes Oct 10, 2024
View reviewed changes
runtime/compiler/z/codegen/UncommonBCDCHKAddressNode.cpp
@@ -62,17 +62,44 @@ UncommonBCDCHKAddressNode::perform()
{
TR::Node* pdOpNode = node->getFirstChild();
TR::Node* oldAddressNode = node->getSecondChild();
TR_ASSERT(pdOpNode && oldAddressNode, "Unexpected null PD opNode or address node under BCDCHK");
TR_ASSERT(oldAddressNode->getNumChildren() == 2, "Expecting 2 children under address node of BCDCHK.");
TR_ASSERT_FATAL_WITH_NODE(node, pdOpNode && oldAddressNode, "Unexpected null PD opNode or address node under BCDCHK");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that this is the old code, but if the intention of this assert is that both oldAddressNode and pdOpNode should be non null, then we should explicitly check that.

runtime/compiler/z/codegen/UncommonBCDCHKAddressNode.cpp

TR::ILOpCodes addressOp = oldAddressNode->getOpCodeValue();
TR_ASSERT((addressOp == TR::aladd || addressOp == TR::aiadd), "Unexpected addressNode opcode");
TR_ASSERT_FATAL_WITH_NODE(node,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to have the expected tree structure comment moved here before the ASSERT to highlight what you are checking here?

runtime/compiler/z/codegen/UncommonBCDCHKAddressNode.cpp
TR::Node* newAddressNode = NULL;
if (oldAddressNode->getOpCodeValue() == TR::aloadi)
{
/* Expected tree structure (probably just loading first array element because offset is 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate why probably?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r30shah r30shah r30shah requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@VermaSh @r30shah