Fix VABS evaluator - #6722
Merged
Merged
Conversation
InlineVectorUnaryOp evaluator was called to generate instruction for VABS for integral type but did not emit VLP instruction to load positive value of the operand for ABS. This commit fixes that and also changes an assert to fatal assert to make sure it is called for the op which is supported. Fixes: eclipse-openj9/openj9#15918 Signed-off-by: Rahil Shah <rahil@ca.ibm.com>
Contributor
Author
|
@joransiu Can I get a review on this change ? |
joransiu
reviewed
Sep 22, 2022
| break; | ||
| default: | ||
| TR_ASSERT(false, "Unary Vector IL evaluation unimplemented for node : %s\n", cg->getDebug()->getName(node)); | ||
| TR_ASSERT_FATAL_WITH_NODE(node, false, "Unary Vector IL evaluation unimplemented for node\n"); |
Contributor
There was a problem hiding this comment.
Is there no way we can print the node name with TR_ASSERT_FATAL_WITH_NODE? That'll be really useful to know if we hit this assert in the future.
Contributor
Author
There was a problem hiding this comment.
TR_ASSERT_FATAL_WITH_NODE actually prints out more useful information. Here is the sample I got when changed the assert to fatal and ran the test that hits it.
Testing Int128Vector ABS
Assertion failed at /root/VectorAPIJDK19Build/openj9-openjdk-jdk19/omr/compiler/z/codegen/OMRTreeEvaluator.cpp:14430: false
VMState: 0x0005ff04
Node 0x3ff8fed2d50 [vabs]: Unary Vector IL evaluation unimplemented for node
...
Node context:
...
n6748n treetop [ 0x3ff8fed2d00] bci=[18,91,684] rc=0 vc=7536 vn=- li=374 udi=- nc=1
n6749n vabsVector128Int32 () [ 0x3ff8fed2d50] bci=[18,91,684] rc=2 vc=7536 vn=- li=374 udi=- nc=1 flg=0x20
n6750n vloadi <array-shadow>[#269 Shadow] [flags 0x80000613 0x0 ] () [ 0x3ff8fed2da0] bci=[7,32,3446] rc=1 vc=7536 vn=- li=374 udi=- nc=1 flg=0x20
n6751n aladd (X>=0 sharedMemory ) [ 0x3ff8fed2df0] bci=[-1,0,112] rc=0 vc=7547 vn=- li=374 udi=- nc=2 flg=0x100
n6752n aload a<parm 1 [I>[#383 Parm] [flags 0xc0000107 0x0 ] (X!=0 sharedMemory ) [ 0x3ff8fed2e40] bci=[7,17,3447] rc=7 vc=7536 vn=- li=374 udi=- nc=0 flg=0x4
n6753n lsub (cannotOverflow ) [ 0x3ff8fed2e90] bci=[-1,0,112] rc=0 vc=7536 vn=- li=374 udi=- nc=2 flg=0x1000
n6754n lshl (cannotOverflow ) [ 0x3ff8fed2ee0] bci=[-1,0,112] rc=0 vc=7536 vn=- li=374 udi=- nc=2 flg=0x1000
n6755n i2l [ 0x3ff8fed2f30] bci=[12,4,3695] rc=0 vc=7536 vn=- li=374 udi=- nc=1
n6776n ==>icall
n8133n iconst 2 [ 0x3ff8de9dde0] bci=[-1,0,112] rc=0 vc=7536 vn=- li=374 udi=- nc=0
n6758n lconst -16 (X!=0 X<=0 ) [ 0x3ff8fed3020] bci=[-1,0,112] rc=7 vc=7536 vn=- li=374 udi=- nc=0 flg=0x204
Contributor
There was a problem hiding this comment.
Oh cool! Haven't seen that before. Changes LGTM.
joransiu
approved these changes
Sep 22, 2022
Contributor
Author
|
Jenkins build zlinux,zos |
Contributor
Author
|
Build passes on LoZ, zOS failure seems like infrastructure issue. Launching z/OS only again. |
Contributor
Author
|
Jenkins build zos |
Contributor
Author
|
@0xdaryl Can you please review and merge this PR? This one fixes the failure seen on nightly builds with ABS vector tests on Z. |
Contributor
Author
|
Jenkins build zos |
Contributor
Author
|
I am unable to get the Jenkins build going on z/OS, it fails due to infrastructure related issue. I did tested out this changes with sanity.openjdk tests with JDK19 where it fails in the nightly build ( |
0xdaryl
approved these changes
Sep 28, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
InlineVectorUnaryOp evaluator was called to generate instruction for VABS for integral type but did not emit VLP instruction to load positive value of the operand for ABS. This commit fixes that and also changes an assert to fatal assert to make sure it is called for the op which is supported.
Fixes: eclipse-openj9/openj9#15918
Signed-off-by: Rahil Shah rahil@ca.ibm.com