Add vblend vector operation #7729
Conversation
gita-omr
requested review from
0xdaryl,
knn-k,
vijaysun-omr and
mstoodle
as code owners
|
Jenkins build all |
|
Fixed build errors. |
|
Jenkins build all |
|
RISC build failed due to unrelated reasons (machine is offline). |
|
What is the type and semantics of the "mask" child? i.e., how does it describe how the the first and second children are blended? |
|
All three arguments have the same number of elements. Mask element is a boolean value. If it's true, corresponding element of the first vector is chosen. If it's false, element from the second argument is chosen. |
gita-omr
changed the title
|
Actually, it's a good question. Let me think what the exact semantic should be. |
What is
If this is meant to be analogous to |
gita-omr
changed the title
|
Updated with expected semantics. |
We have |
I see. It's unfortunate that the order of the operands differs from those of |
|
Thanks for doing this @gita-omr. Did you find out the reason that the vbitselect opcode had a strange extra operand? |
This is a common meaning for vector blend operations and that's how corresponding instructions work at least on Intel and Power. Essentially, we blend second vector into the first one based on the third vector or mask. |
|
To reviewers: please let me know if you have any other questions. |
compiler/il/VectorOperations.enum
Outdated
| /* .reverseBranchOperation = */ TR::vBadOperation, \ | ||
| /* .booleanCompareOpCode = */ TR::BadILOp, \ | ||
| /* .ifCompareOpCode = */ TR::BadILOp, \ | ||
| /* .description = vector blend witn mask operator */ \ |
There was a problem hiding this comment.
It would be helpful if your clarification on semantics were documented here in the code rather than in a PR comment.
compiler/il/VectorOperations.enum
Outdated
| /* .reverseBranchOperation = */ TR::vBadOperation, \ | ||
| /* .booleanCompareOpCode = */ TR::BadILOp, \ | ||
| /* .ifCompareOpCode = */ TR::BadILOp, \ | ||
| /* .description = vector blend witn mask operator */ \ |
|
As new opcodes are normally discussed at OMR Architecture Meetings, I'd like at least one committer other than me to formally approve this before merging. |
|
Addressed comments above. |
compiler/il/VectorOperations.enum
Outdated
| /* .description = vector blend operator */ \ | ||
| /* blends second vector into first based on mask (third child) */ \ | ||
| /* if lane is not set in the mask the value from the first vector is chosen */ \ | ||
| /* the lane from the second vector is chosen otherwise */ |
There was a problem hiding this comment.
I'm sorry to nit about something like this, but the commenting style is a bit awkward. I'm not clear why you use \ at the end of each comment line except the last one, and why each line has its own comment.
Something like you have for the vcompressbits opcode seems more visually appealing (to me at least).
There was a problem hiding this comment.
I took it from here:
omr/compiler/il/OMROpcodes.enum
Line 7204 in 0dcc239
There was a problem hiding this comment.
But you are right vcompressbits looks better. Let me change.
|
Improved opcode description comment. |
|
Thanks. Please squash commits and I can re-launch testing. I'd appreciate a +1 from another committer please. |
|
Squashed. |
|
Jenkins build all |
There was a problem hiding this comment.
I'm not sure that vblend should have ILProp2::Select. To me, that suggests that it works like an xselect opcode (where x is one of the basic data types), but there are a number of differences between them:
xselectchooses based on a single zero/nonzero integer vs. the vector mask thatvblenduses;- the result of
xselectis the result of one of the two value children, whereas the result ofvblendcan be a combination of the two; - the operand indicating which choice(s) to make is the first child for
xselect, but the third child forvblend; and - the boolean choice is interpreted in the opposite order with respect to the two values, with the first of them corresponding to true for
xselectbut false forvblend.
This also applies to vbitselect (though the specifics of the differences won't be exactly the same). At the moment, vbitselect appears to be the only operation other than xselect that has the ILProp2::Select property.
I can easily imagine some logic trying to operate generically on the various xselect opcodes and trying to identify them simply by calling isSelect() on the opcode. The ILProp2::Select property on vblend (and vbitselect) would thoroughly confuse any such logic. However, I looked and I was unable to find any existing place where this happens without an additional condition that the select node be address-typed, integral-typed, or have an opcode that also satisfies isInteger(). So I don't think it could cause a functional issue with the current uses of isSelect(), but it could potentially be a footgun in the future.
|
Thanks @jdmpapin for the great observation in #7729 (review). I agree that both For now, I will just remove |
- similar to vbitselect but works on lanes and under control of the mask (third child) - if lane is not set in the mask the value from the first vector is chosen - the lane from the second vector is chosen otherwise
|
Removed We can discuss |
|
Jenkins build all |
|
The Eclipse CI infrastructure has been down for a few days (no ETA as far as I know). @gita-omr, can you manually verify this PR builds and passes the OMR tests on either X or P please and confirm here. Thanks. |
I built this PR on Intel and Power. I also ran OMR tests on Power and only |
|
CI infrastructure is down, but I am satisfied with the offline testing done on this. |
Uh oh!
There was an error while loading. Please reload this page.