Repository navigation
Add xarch blsmsk - #66561
Add xarch blsmsk#66561
blsmsk#66561Conversation
ghost
added
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsThis adds a lowering for the pattern XOR(x, x - 1) to the GetMaskUpToLowestSetSit hwintrinsic. /cc @dotnet/jit-contrib @AndyAyersMS
|
|
I am personally not against such changes, but I wish we had a DSL or much simpler way to express such transformations - 100 LOC for a pattern which is never met in the codebase sounds too much. |
860a8e6 to
1862513
Compare
|
Can you add a test, verifying this optimization? |
|
Test coverage was added in the |
|
Oh sorry, I overread that 👍🏼 |
|
@Wraith2 could you please rebase this PR? |
JulieLeeMSFT
added
the
needs-author-action
2a8f8f8 to
26404d1
Compare
ghost
removed
the
needs-author-action
|
rebased, lets see what CI thinks. |
|
Looks fixed. |
|
@EgorBo rebased, current failures don't look related. |
|
@Wraith2 sorry for the delay, I cloned your branch to try locally and see this:
do you have any idea why it emitted that failfast helper? |
|
Ah, nvm, it's |
| INST3(blsi, "blsi", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xF3), Resets_OF | Writes_SF | Writes_ZF | Undefined_AF | Undefined_PF | Writes_CF | INS_Flags_IsDstDstSrcAVXInstruction) // Extract Lowest Set Isolated Bit | ||
| INST3(blsmsk, "blsmsk", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xF3), INS_Flags_IsDstDstSrcAVXInstruction) // Get Mask Up to Lowest Set Bit | ||
| INST3(blsi, "blsi", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xF3), Resets_OF | Writes_SF | Writes_ZF | Undefined_AF | Undefined_PF | Writes_CF | INS_Flags_IsDstDstSrcAVXInstruction) // Extract Lowest Set Isolated Bit | ||
| INST3(blsmsk, "blsmsk", IUM_WR, BAD_CODE, BAD_CODE, SSE38(0xF3), Resets_OF | Writes_SF | Writes_ZF | Undefined_AF | Undefined_PF | Writes_CF | INS_Flags_IsDstDstSrcAVXInstruction) // Get Mask Up to Lowest Set Bit |
There was a problem hiding this comment.
Are you sure Writes_ZF is correct? shouldn't it be Resets_ZF?
There was a problem hiding this comment.
The intel manual says:
SF is updated based on the result. CF is set if the source if zero. ZF and OF flags are cleared. AF and PF flag are
undefined.
There was a problem hiding this comment.
ZF and OF flags are cleared so it means it has to be Resets_ZF rather than Writes_ZF, right?
There was a problem hiding this comment.
I'm not sure i'm qualified to answer. The manual lists the pseudo code for the operation as:
temp ← (SRC-1) bitwiseAND ( SRC );
SF ← temp[OperandSize -1];
ZF ← (temp = 0);
IF SRC = 0
CF ← 1;
ELSE
CF ← 0;
FI
DEST ← temp;
which to me writes to SF ZF and CF, additionally it says that OF is cleared which I read as being reset.
There was a problem hiding this comment.
so is ZF, even from your pseudocode it looks like it's always cleared
And the doc says ZF and OF flags are cleared.
| if (((addOp2->gtFlags & GTF_SET_FLAGS) != 0) || ((op2->gtFlags & GTF_SET_FLAGS) != 0) || | ||
| ((xorNode->gtFlags & GTF_SET_FLAGS) != 0)) |
There was a problem hiding this comment.
Correct if I am wrong but you only need to check xorNode here, e.g. if (xorNode->gtSetFlags())
There was a problem hiding this comment.
Possibly, I copied the check used in TryLowerAndOpToResetLowestSetBit Would ci pick up if it doesn't work?
|
/azp run Fuzzlyn |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@Wraith2 thanks! 🙂 |
This adds a lowering for the pattern XOR(x, x - 1) to the GetMaskUpToLowestSetSit hwintrinsic.
There are no uses of this pattern in the runtime. SPMI replay is clean. Test coverage is added by #66193 and this PR is opened for completeness of the simple BMI1 intrinsic.
/cc @dotnet/jit-contrib @AndyAyersMS