Update BCDCHK uncommoning for off-heap #20302

VermaSh · 2024-10-04T15:51:57Z

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

VermaSh · 2024-10-04T15:53:29Z

cc: @r30shah @zl-wang @rmnattas

VermaSh · 2024-10-04T15:58:38Z

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 · 2024-10-07T19:56:47Z

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

VermaSh · 2024-10-10T19:39:36Z

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

r30shah · 2024-10-10T20:00:38Z

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");


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.

r30shah · 2024-10-10T20:05:14Z

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,


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

r30shah · 2024-10-10T20:06:15Z

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)


Can you elaborate why probably?

Sorry, forgot to take that out. That tree structure is possible only when accessing first array element so "probably" is not needed.

VermaSh · 2024-10-11T16:15:31Z

@r30shah I have addressed your suggestions in my latest commit. Launched another off-heap build to confirm everything is still good.

r30shah · 2024-10-11T18:15:19Z

runtime/compiler/z/codegen/UncommonBCDCHKAddressNode.cpp

-            method == TR::com_ibm_dataaccess_PackedDecimal_remainderPackedDecimal_||
-            method == TR::com_ibm_dataaccess_PackedDecimal_shiftLeftPackedDecimal_ ||
-            method == TR::com_ibm_dataaccess_PackedDecimal_shiftRightPackedDecimal_)
+         TR::Node* oldAddressNode = node->getSecondChild();


TR::BCDCHK has unspecified child properties. Are we guaranteed to have second child?

I have updated the if condition to enter only if the node BCDCHK has two children.

It was more of a question, but I do not mind the conservative root you took. Do we need to have oldAddressNode here ? I only see it being used inside the if block.

r30shah · 2024-10-11T18:19:50Z

runtime/compiler/z/codegen/UncommonBCDCHKAddressNode.cpp

+             *         array_element_offset
+             */
+            TR_ASSERT_FATAL_WITH_NODE(node,
+                                      oldAddressNode->isDataAddrPointer() && oldAddressNode->getNumChildren() == 1


Please add brackets here

Suggested change

oldAddressNode->isDataAddrPointer() && oldAddressNode->getNumChildren() == 1

(oldAddressNode->isDataAddrPointer() && oldAddressNode->getNumChildren() == 1)

r30shah · 2024-10-11T18:21:54Z

runtime/compiler/z/codegen/UncommonBCDCHKAddressNode.cpp

+                                      oldAddressNode->isDataAddrPointer() && oldAddressNode->getNumChildren() == 1
+                                          || ((addressOp == TR::aladd || addressOp == TR::aiadd) && oldAddressNode->getNumChildren() == 2),
+                                      "Expecting either a dataAddrPointer node with 1 child or a aladd/aiadd with 2 children under address node of BCDCHK."
+                                      "But the address (opCode %s) node had %d children and dataAddrPointer flag set to %d\n", oldAddressNode->getOpCode().getName(), oldAddressNode->getNumChildren(), oldAddressNode->isDataAddrPointer());


Suggested change

"But the address (opCode %s) node had %d children and dataAddrPointer flag set to %d\n", oldAddressNode->getOpCode().getName(), oldAddressNode->getNumChildren(), oldAddressNode->isDataAddrPointer());

"But the address node %s had %d children and dataAddrPointer flag set to %s\n", oldAddressNode->getOpCode().getName(), oldAddressNode->getNumChildren(), oldAddressNode->isDataAddrPointer() ? "true" : "false");

r30shah · 2024-10-11T18:22:30Z

runtime/compiler/z/codegen/UncommonBCDCHKAddressNode.cpp

+                                      "But the address (opCode %s) node had %d children and dataAddrPointer flag set to %d\n", oldAddressNode->getOpCode().getName(), oldAddressNode->getNumChildren(), oldAddressNode->isDataAddrPointer());
+            TR::Node* newAddressNode = NULL;
+            if (oldAddressNode->getOpCodeValue() == TR::aloadi)
+               { // handles expected tree structure 2


Will leave it up to your preference, but I would put the comment in next line.

r30shah · 2024-10-11T18:23:11Z

runtime/compiler/z/codegen/UncommonBCDCHKAddressNode.cpp

               }
+            else
+               { // handles expected tree structure 1 and 3


Similar comment as for line 91

VermaSh · 2024-10-17T03:52:19Z

Launched another off-heap personal build for verification.

VermaSh · 2024-10-17T14:37:06Z

@r30shah This is ready for another review.

r30shah

LGTM, giving soft approval, will wait for you to push change that moved getting oldAddressNode in the if block and then launch the sanity tests. I am satisfied with the internal testing you have done.

r30shah · 2024-10-17T14:53:55Z

runtime/compiler/z/codegen/UncommonBCDCHKAddressNode.cpp

-            method == TR::com_ibm_dataaccess_PackedDecimal_remainderPackedDecimal_||
-            method == TR::com_ibm_dataaccess_PackedDecimal_shiftLeftPackedDecimal_ ||
-            method == TR::com_ibm_dataaccess_PackedDecimal_shiftRightPackedDecimal_)
+         TR::Node* oldAddressNode = node->getSecondChild();


It was more of a question, but I do not mind the conservative root you took. Do we need to have oldAddressNode here ? I only see it being used inside the if block.

r30shah · 2024-10-17T14:59:50Z

runtime/compiler/z/codegen/UncommonBCDCHKAddressNode.cpp

         {
         TR::RecognizedMethod method = node->getSymbol()->getResolvedMethodSymbol()->getRecognizedMethod();
+         TR::Node* oldAddressNode = node->getSecondChild();
         if(method == TR::com_ibm_dataaccess_DecimalData_convertIntegerToPackedDecimal_ ||


I do not want to stretch this PR too much as it is fixing the functional issue, and following question is related to code that already exists so we can probably in future change this.
Instead of having a large if condition check, we should take out the check for recog. DAA method to a separate function and have a switch case in that function, that will make this code more readable.

Again, suggestion as it is something I observed in the PR.

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 · 2024-10-17T15:21:23Z

We don't need oldAddressNode outside if block, it was left from previous iteration of my changes. I have moved inside the if block.

r30shah · 2024-10-17T15:22:45Z

Jenkins test sanity zlinux jdk21,jdk11

VermaSh marked this pull request as draft October 4, 2024 15:54

VermaSh force-pushed the off-heap_daa_failure_fix branch 2 times, most recently from 8e4f933 to c934487 Compare October 4, 2024 19:04

VermaSh marked this pull request as ready for review October 10, 2024 19:39

VermaSh changed the title ~~WIP: Update BCDCHK uncommoning for off-heap~~ Update BCDCHK uncommoning for off-heap Oct 10, 2024

r30shah requested changes Oct 10, 2024

View reviewed changes

VermaSh force-pushed the off-heap_daa_failure_fix branch from c934487 to 0c75203 Compare October 11, 2024 16:12

r30shah requested changes Oct 11, 2024

View reviewed changes

VermaSh force-pushed the off-heap_daa_failure_fix branch from 0c75203 to 86f3da4 Compare October 17, 2024 03:45

r30shah approved these changes Oct 17, 2024

View reviewed changes

Update BCDCHK uncommoning for off-heap

aa6d3d9

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 force-pushed the off-heap_daa_failure_fix branch from 86f3da4 to aa6d3d9 Compare October 17, 2024 15:19

r30shah approved these changes Oct 17, 2024

View reviewed changes

r30shah merged commit 89c4ba2 into eclipse-openj9:master Oct 17, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update BCDCHK uncommoning for off-heap #20302

Update BCDCHK uncommoning for off-heap #20302

VermaSh commented Oct 4, 2024

VermaSh commented Oct 4, 2024

VermaSh commented Oct 4, 2024 •

edited

Loading

VermaSh commented Oct 7, 2024 •

edited

Loading

VermaSh commented Oct 10, 2024

r30shah Oct 10, 2024

r30shah Oct 10, 2024

r30shah Oct 10, 2024

VermaSh Oct 11, 2024

VermaSh commented Oct 11, 2024 •

edited

Loading

r30shah Oct 11, 2024

VermaSh Oct 17, 2024

r30shah Oct 17, 2024

r30shah Oct 11, 2024

r30shah Oct 11, 2024

r30shah Oct 11, 2024

r30shah Oct 11, 2024

VermaSh commented Oct 17, 2024 •

edited

Loading

VermaSh commented Oct 17, 2024

r30shah left a comment

r30shah Oct 17, 2024

r30shah Oct 17, 2024

VermaSh commented Oct 17, 2024

r30shah commented Oct 17, 2024

	oldAddressNode->isDataAddrPointer() && oldAddressNode->getNumChildren() == 1
	(oldAddressNode->isDataAddrPointer() && oldAddressNode->getNumChildren() == 1)

	"But the address (opCode %s) node had %d children and dataAddrPointer flag set to %d\n", oldAddressNode->getOpCode().getName(), oldAddressNode->getNumChildren(), oldAddressNode->isDataAddrPointer());
	"But the address node %s had %d children and dataAddrPointer flag set to %s\n", oldAddressNode->getOpCode().getName(), oldAddressNode->getNumChildren(), oldAddressNode->isDataAddrPointer() ? "true" : "false");

Update BCDCHK uncommoning for off-heap #20302

Update BCDCHK uncommoning for off-heap #20302

Conversation

VermaSh commented Oct 4, 2024

VermaSh commented Oct 4, 2024

VermaSh commented Oct 4, 2024 • edited Loading

VermaSh commented Oct 7, 2024 • edited Loading

VermaSh commented Oct 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VermaSh commented Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VermaSh commented Oct 17, 2024 • edited Loading

VermaSh commented Oct 17, 2024

r30shah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VermaSh commented Oct 17, 2024

r30shah commented Oct 17, 2024

VermaSh commented Oct 4, 2024 •

edited

Loading

VermaSh commented Oct 7, 2024 •

edited

Loading

VermaSh commented Oct 11, 2024 •

edited

Loading

VermaSh commented Oct 17, 2024 •

edited

Loading