Standardize vbitselect operand ordering across platforms - #7935
Conversation
|
@r30shah when I was running the unit tests, I got a failure in vbitselect tests and created the #7910 issue. When investigating, I find that the result values are the opposite of what the tests expected and can be fixed by swapping the two source registers. |
| TR::Register *returnVecReg = cg->allocateRegister(TR_VRF); | ||
|
|
||
| generateVRReInstruction(cg, TR::InstOpCode::VSEL, node, returnVecReg, firstVecReg, secondVecReg, vecSelectReg, 0, | ||
| generateVRReInstruction(cg, TR::InstOpCode::VSEL, node, returnVecReg, secondVecReg, firstVecReg, vecSelectReg, 0, |
There was a problem hiding this comment.
I think it is incorrect to drive the evaluator behavior based on the test outcome. It should be driven by the vbitselect operation - Does the operation requires us to select the bits from first child for the bits set to one in third child and use the bits from second child for the bits set to 0.
To me it seems counter-intuitive to swap the children.
Please check what the opcode is suppose to do and fix accordingly.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
I updated the implementation to match other ternary evaluators
692f7cc to
ed0f6d8
Compare
ed0f6d8 to
b08699c
Compare
b08699c to
95e6ad1
Compare
ehsankianifar
changed the title
|
@gita-omr I updated the vselectbit opcode implementation on all platforms as we discussed earlier today. Could you please take a look and trigger the jenkins tests? |
Standardized the operand ordering for the vbitselect opcode to match the convention used by other select opcodes across all supported platforms. In the new schema, child 1 is the selector, child 2 is the value selected when the selector is set, and child 3 is the value selected when the selector is unset. signed-off-by: Ehsan Kiani Far <ehsan.kianifar@gmail.com>
95e6ad1 to
340ffa3
Compare
|
Jenkins build all |
|
I see failures in https://ci.eclipse.org/omr/job/PullRequest-linux_ppc-64_le_gcc/4919/ , https://ci.eclipse.org/omr/job/PullRequest-linux_riscv64/450/ and, https://ci.eclipse.org/omr/job/PullRequest-osx_x86-64/4089/ but the failure does't seem relevant to my changes. |
|
Hi @BradleyWood |
|
Reviewed all changes except Intel and they look good to me. |
BradleyWood
left a comment
BradleyWood
left a comment
There was a problem hiding this comment.
I reviewed and approve from an x86 perspective.
|
Hi @hzongaro, |
I'll rely on the reviews of the various platform experts rather than rereviewing myself. @r30shah, have you had a chance to verify that your comment has been addressed? If so, and the pull request looks good to you, may I ask you to indicate your approval? @gita-omr, I see that you indicated above that the changes looked good to you - may I ask you to mark your review as approved? |
r30shah
left a comment
r30shah
left a comment
There was a problem hiding this comment.
LGTM - Thanks @ehsankianifar for taking this up and fix on all platform.
|
Failure running linux_ppc-64_le_gcc testing appears to be due to known issue #6571. Merging. |
7 participants
Standardized the operand ordering for the vbitselect opcode to match
the convention used by other select opcodes across all supported
platforms. In the new schema, child 1 is the selector, child 2 is the
value selected when the selector is set, and child 3 is the value
selected when the selector is unset.