Remove node refCount decrement in DAA evaluators #16287

VermaSh · 2022-11-08T13:30:01Z

No need to decrement reference count of address node in
pdloadVectorEvaluatorHelper, pdstoreVectorEvaluatorHelper and
zdstoreiVectorEvaluatorHelper evaluators because that happens in
MemoryReference(TR::Node * rootLoadOrStore...) on
rootLoadOrStore->getOpCode().isIndirect() path.

Comments in those evaluators have also been updated to reflect this.

Signed-off-by: Shubham Verma shubhamv.sv@gmail.com

VermaSh · 2022-11-08T13:36:12Z

@r30shah My personal build (zlinux and zOS) passed without failures. However, I don't think we run systemstest in those builds. So, going to run it locally to verify functional correctness of these changes.

r30shah · 2022-11-08T14:26:03Z

@VermaSh When you launch a personal build on internal jenkins, for test target, add following two to cover the system tests.
sanity.system , extended.system this will execute the tests you wanted to get executed.

Also, would it be possible to have a unit test for this? I think the failing tree trace where we see null address node and suspect it is pdload could be something like this.

pdstorei
   pdload

Though I am still scratching my head around how we could end with this node?

r30shah

Looks good to me, @VermaSh Can you check and verify the code path that is taken to generate memory reference and VSII instruction does handle pdload ?

VermaSh · 2022-11-22T19:13:17Z

Both sanity.system and extended.system tests passed view/OpenJ9 - Personal/job/Pipeline-Build-Test-Personal/14847/.

Also, we do take care of pdload. The address node is evaluated in [1-2]. The path through VSII instruction doesn't do much because the memory reference doesn't have an index register [3]. It doesn't look like we ever will because TR::MemoryReference::create(cg, node) [4] takes [5] path; to generate index reg we need to go through [6].

TODOs:

Update comment in the evaluators
Setup a meeting to figure out the path that could have lead to this being generated [7]

[1] https://github.com/eclipse/omr/blob/26e52210d25d5b3cd84b1573d4060ef7b663303e/compiler/z/codegen/OMRMemoryReference.cpp#L527 "Populate memory reference"
[2] https://github.com/eclipse/omr/blob/26e52210d25d5b3cd84b1573d4060ef7b663303e/compiler/z/codegen/OMRMemoryReference.cpp#L1819-L1827 "Evaluate the address node (base register)"
[3] https://github.com/eclipse/omr/blob/310f0efc871923145d85dd1f450b6a08dd11ef35/compiler/z/codegen/OMRMemoryReference.cpp#L2265 "separateIndexRegister(...)"
[4]

openj9/runtime/compiler/z/codegen/J9BCDTreeEvaluator.cpp

Line 3521 in 55c52ff

TR::MemoryReference* sourceMR = TR::MemoryReference::create(cg, node);

"pdloadVectorEvaluatorHelper(...)"
[5] https://github.com/eclipse/omr/blob/26e52210d25d5b3cd84b1573d4060ef7b663303e/compiler/z/codegen/OMRMemoryReference.cpp#L470 "Memory Reference canUseRX path"
[6] https://github.com/eclipse/omr/blob/26e52210d25d5b3cd84b1573d4060ef7b663303e/compiler/z/codegen/OMRMemoryReference.cpp#L692 "Memory Reference canUseIndexReg path"
[7] https://github.ibm.com/runtimes/openj9-jit-z/issues/737 "Internal issue tracking this"

VermaSh · 2022-11-29T00:03:20Z

Launched a personal build to verify the changes view/OpenJ9 - Personal/job/Pipeline-Build-Test-Personal/14942/

VermaSh · 2022-11-29T21:23:38Z

~~@r30shah this is ready for another review~~ There was an infra failure near the end of my previous personal build. I have launched another personal build with the squashed commits to verify that we are still good. view/OpenJ9 - Personal/job/Pipeline-Build-Test-Personal/14958/

r30shah · 2022-11-30T13:34:35Z

@VermaSh Is this ready for review?

VermaSh · 2022-11-30T15:42:28Z

@r30shah Yup, this is ready for review. There were some failures in my personal build but none of them were related to DAA. The failures were a mix of java.lang.NullPointerException, java.io.IOException: Not a directory and java.lang.AssertionError: main process did not initialize attach API. But highly unlikely that they are related to this PR. I'll open another issue to track those failures.

r30shah

Change looks good to me. Just minor change request to have original comment back.

r30shah · 2022-11-30T16:38:39Z

runtime/compiler/z/codegen/J9BCDTreeEvaluator.cpp

-   // register by emitting an LA instruction. If there's a need for large displacement adjustment,
-   // LAY will be emitted instead.
-   TR::MemoryReference * targetMR = TR::MemoryReference::create(cg, node);;
+   // No need to evaluate the address node (first child) of the pdstorei.


I think originally the second comment was meant to be used for generateVSIInstruction , it does not meant to say that it will evaluate the address node. As comment says, the VSI instruction generation will separate out the index register from the memory reference. Your current comment enhances what we put in first place, but we should also keep the second comment (May be before generate*).

The call to create memory reference in those 3 evaluators goes through [1] which doesn't create an index register. Index register doesn't get generated on the path to separateIndexRegister(...) either so that's why the comment about generateVSIInstruction didn't feel relevant here.

[1] https://github.com/eclipse/omr/blob/26e52210d25d5b3cd84b1573d4060ef7b663303e/compiler/z/codegen/OMRMemoryReference.cpp#L470 "Memory Reference canUseRX path"
[2] https://github.com/eclipse/omr/blob/310f0efc871923145d85dd1f450b6a08dd11ef35/compiler/z/codegen/OMRMemoryReference.cpp#L2265 "separateIndexRegister(...)"

generateVSIInstruction calls to enforceSSFormatLimits on the memory reference, which takes care of translating memory reference that needs all three index register , base register and displacement to the SS format memory reference generating necessary LA or LAY instruction.

The call to create memory reference in those 3 evaluators goes through [1] which doesn't create an index register.

I do not think that is true. It is common constructor that is uses to generate memory reference for all instruction. If you take a look at the code that would be executed for indirect load or store [2], you will see that it first tries to generate tryBaseIndexDispl for the memory reference which would allocate base register, index register and set displacement value for the memory reference.

[1]. https://github.com/eclipse/omr/blob/310f0efc871923145d85dd1f450b6a08dd11ef35/compiler/z/codegen/OMRMemoryReference.cpp#L2054-L2067
[2]. https://github.com/eclipse/omr/blob/310f0efc871923145d85dd1f450b6a08dd11ef35/compiler/z/codegen/OMRMemoryReference.cpp#L1610

ah true true, you are right. I'll update the PR

r30shah

Looks good to me, @VermaSh please confirm if you have executed system tests (Extended and sanity) with this changes or not.

@joransiu Can I please request you to launch tests and review/merge this PR?

runtime/compiler/z/codegen/J9BCDTreeEvaluator.cpp

No need to decrement reference count of address node in `pdloadVectorEvaluatorHelper`, `pdstoreVectorEvaluatorHelper` and `zdstoreiVectorEvaluatorHelper` evaluators because that happens in `MemoryReference(TR::Node * rootLoadOrStore...)` on `rootLoadOrStore->getOpCode().isIndirect()` path. Comments in those evaluators have also been updated to reflect this. Signed-off-by: Shubham Verma <shubhamv.sv@gmail.com>

VermaSh · 2022-12-01T15:53:24Z

@r30shah Yup both sanity.system , extended.system passed without any failures

joransiu · 2022-12-01T19:04:20Z

jenkins test sanity zlinux jdk8,jdk11,jdk17

joransiu · 2022-12-02T17:26:13Z

jenkins test sanity zlinux jdk8

r30shah approved these changes Nov 8, 2022

View reviewed changes

VermaSh force-pushed the daa_pdload_improvement branch 3 times, most recently from 77a55e2 to e2dd661 Compare November 28, 2022 23:57

VermaSh force-pushed the daa_pdload_improvement branch 2 times, most recently from 6b0f3b9 to f5e9398 Compare November 29, 2022 21:21

r30shah requested changes Nov 30, 2022

View reviewed changes

VermaSh force-pushed the daa_pdload_improvement branch from f5e9398 to ee8f73e Compare December 1, 2022 01:01

r30shah approved these changes Dec 1, 2022

View reviewed changes

joransiu requested changes Dec 1, 2022

View reviewed changes

runtime/compiler/z/codegen/J9BCDTreeEvaluator.cpp Outdated Show resolved Hide resolved

VermaSh force-pushed the daa_pdload_improvement branch from ee8f73e to 5ed7754 Compare December 1, 2022 15:51

VermaSh changed the title ~~Decrement ref count only if evaluating pdloadi~~ Remove node refCount decrement in DAA evaluators Dec 1, 2022

joransiu approved these changes Dec 1, 2022

View reviewed changes

joransiu added the comp:jit label Dec 1, 2022

joransiu merged commit d1e10d8 into eclipse-openj9:master Dec 2, 2022

VermaSh deleted the daa_pdload_improvement branch December 5, 2022 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove node refCount decrement in DAA evaluators #16287

Remove node refCount decrement in DAA evaluators #16287

VermaSh commented Nov 8, 2022 •

edited

Loading

VermaSh commented Nov 8, 2022

r30shah commented Nov 8, 2022 •

edited

Loading

r30shah left a comment

VermaSh commented Nov 22, 2022 •

edited

Loading

VermaSh commented Nov 29, 2022

VermaSh commented Nov 29, 2022 •

edited

Loading

r30shah commented Nov 30, 2022

VermaSh commented Nov 30, 2022

r30shah left a comment

r30shah Nov 30, 2022

VermaSh Nov 30, 2022

r30shah Nov 30, 2022

VermaSh Dec 1, 2022

r30shah left a comment

VermaSh commented Dec 1, 2022

joransiu commented Dec 1, 2022

joransiu commented Dec 2, 2022

Remove node refCount decrement in DAA evaluators #16287

Remove node refCount decrement in DAA evaluators #16287

Conversation

VermaSh commented Nov 8, 2022 • edited Loading

VermaSh commented Nov 8, 2022

r30shah commented Nov 8, 2022 • edited Loading

r30shah left a comment

Choose a reason for hiding this comment

VermaSh commented Nov 22, 2022 • edited Loading

VermaSh commented Nov 29, 2022

VermaSh commented Nov 29, 2022 • edited Loading

r30shah commented Nov 30, 2022

VermaSh commented Nov 30, 2022

r30shah left a comment

Choose a reason for hiding this comment

r30shah Nov 30, 2022

Choose a reason for hiding this comment

VermaSh Nov 30, 2022

Choose a reason for hiding this comment

r30shah Nov 30, 2022

Choose a reason for hiding this comment

VermaSh Dec 1, 2022

Choose a reason for hiding this comment

r30shah left a comment

Choose a reason for hiding this comment

VermaSh commented Dec 1, 2022

joransiu commented Dec 1, 2022

joransiu commented Dec 2, 2022

VermaSh commented Nov 8, 2022 •

edited

Loading

r30shah commented Nov 8, 2022 •

edited

Loading

VermaSh commented Nov 22, 2022 •

edited

Loading

VermaSh commented Nov 29, 2022 •

edited

Loading