Add missing checks to convert store to load #19810
Conversation
|
For reference, we were failing because we were missing call to [1] |
| createIndexOffsetTree(TR::Compilation *comp, bool is64bit, TR::Node *indexNode, int multiply) | ||
| { | ||
| TR::Node *c1 = createBytesFromElement(comp, is64bit, indexNode, multiply); | ||
| return TR::TransformUtil::generateConvertArrayElementIndexToOffsetTrees(comp, c1, NULL, multiply); |
There was a problem hiding this comment.
Looking at the method generateConvertArrayElementIndexToOffsetTrees it takes int32_t not int. Should we fix this ?
There was a problem hiding this comment.
While comparing this change to pre off-heap code I realized that we don't need to call generateConvertArrayElementIndexToOffsetTrees so that should take care of the type miss match.
|
Local testing passed without any failure. Comparing the failing and passing logs to get the trees responsible for the failure. |
These checks were accidentally removed in `Update IdiomRecognitionUtils.cpp to use new TransformUtil APIs` commit. Signed-off-by: Shubham Verma <shubhamv.sv@gmail.com>
|
This doesn't fix #19666 but still needed because it fixes index node reference count bug. The index node ends up with reference count higher than actual number of references resulting in a register being kept alive longer than it should. The original code duplicated the index node before using it in new array address trees, the duplication was accidentally removed during the code refactoring for off-heap changes. While going through the off-heap change I noticed that the original code was subtracting - array header size whereas we are adding array header size. I am in the process of updating the |
|
WIP PR for OMR dependency eclipse-omr/omr#7405 |
|
be careful with that though: @rmnattas once pointed it out to me that the subtraction is needed because caller added it already. in off-heap case, you access elements with dataAddr pointer such that you need to effectively make arrayBaseOffset as zero (so that subtraction is needed). |
|
@zl-wang I am not removing the subtraction. It wasn't really a subtraction actually, we were adding header size to the offset by subtracting negative header size from it. The subtraction got replaced by addition so this change just reverts it back to subtraction. This change applies only to non off-heap path. |
|
Removing my second commit to remove OMR dependency so that this can be merged sooner as my OMR change is causing failures. This change on it's own fixes #19666, I'll launch a personal build to verify this. |
|
Personal build without my OMR changes: https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/23174/ |
There was a problem hiding this comment.
Checking out this change alone (I do think what Julian mentioned could be a issue needs to be checked at for the OMR PR), Comparing changes in this PR with the Utils without changes added for supporting off-heap, it looks good to me.
@zl-wang Appreciate if you can also review the changes.
|
jenkins test sanity.functional all jdk11,jdk21 |
|
My personal build passed without failures so removing WIP tag. |
VermaSh
changed the title
|
@VermaSh Can you verify if failures on windows are related or not ? |
|
@r30shah x builds were aborted because all nodes were offline |
|
jenkins test sanity.functional win jdk11,jdk21 |
|
Based on the testing done by Shubham and finished tests on other platform, I do not think this will cause any issue on windows, and we can merge this safely. @VermaSh This one resolves the blocker right ? Can we update the Commit message to reflect that ? @zl-wang Ping to see if you have any recommendation for Shubham's PR reverting the checks that was removed accidentally. |
|
Yup, from my testing this does fix failure . I have updated the PR description to reflect that. |
|
All windows machines are still offline because they can't access github.com (issue). |
|
As changes are approved and testing has done to satisfactory level, merging this change. |
These checks were accidentally removed in
Update IdiomRecognitionUtils.cpp to use new TransformUtil APIscommit.Fixes: #19666