x86: Fixes for vector masking helpers - #7727
Conversation
|
jenkins build all |
BradleyWood
added
comp:jitbuilder
arch:x86
bug
comp:compiler
and removed
comp:jitbuilder
labels
0xdaryl
left a comment
0xdaryl
left a comment
There was a problem hiding this comment.
While the changes seem reasonable on their own, I don't know what problems they are addressing. Can you add more detail to your commit messages please? Thanks.
|
I have updated the commit messages. @hzongaro Would you mind taking a look? |
hzongaro
left a comment
hzongaro
left a comment
There was a problem hiding this comment.
I think the changes look good. There are a couple of typos in the commit message for the "x86: Fixes for vector masking helpers" commit:
"left-land side" -> "left-hand side"
"a. Perfrom" -> "a. Perform"
"I have rewritten this logic to fix logical issues simplify into 3 cases" -> "I have simpified the logic into three cases", perhaps?
This is outside the scope of this change, but may I ask you to consider adding comments to OMR::X86::TreeEvaluator::arrayToVectorMaskHelper in a follow up pull request? It will certainly help anyone who is not regularly immersed in vector operations to understand what it's doing.
Finally, is it possible to add TRIL tests for b2m, s2m, i2m and l2m? I have no idea what's supported there in terms of vector operations.
This commit fixes issues with merging vectors in masked binary operations. The left-hand side of the binary operation should be moved into the result register whether native masking is supported or not. I have simplified this logic into three cases. 1. Native masking is supported but lane-size is mismatched. a. Perform binary operation as if it is unmasked. b. Call merge helper. 2. Masking is supported. a. Generate binary instruction under write mask. 3. Masking is not supported. a. Copy mask into tmp register and perform binary op. b. Call merge helper. Signed-off-by: Bradley Wood <bradley.wood@ibm.com>
When converting a boolean array to vector mask, we support boolean arrays up to 64 elements. In many cases, the number of elements is less than 16 and therefore fits into a general-purpose register. When copying the array into a vector register, we need to check if the array is stored in general-purpose register and move it into a vector register, not whether its node is an integer. Signed-off-by: Bradley Wood <bradley.wood@ibm.com>
|
Updated commit messages.
May as well do it now. I added doxygen-style comment and add comments to explain the instruction sequence for each of relevant execution paths (AVX-512 native masking vs non-native masking).
I have tested these opcodes through openjdk tests. I am working on TRIL infrastructure for mask operations on another branch. Those tests require support of the complementing opcodes, such as m2v, m2b, m2s, m2i, m2l. |
|
jenkins build all |
Signed-off-by: Bradley Wood <bradley.wood@ibm.com>
|
Jenkins build all |
|
x86 testing has passed. No need to wait for the linux_riscv64 testing to complete. Merging. |
3 participants
No description provided.