-
Notifications
You must be signed in to change notification settings - Fork 161
[CIR][Lowering] Fix Vector Comparison Lowering with -fno-signed-char/unsigned operand #1770
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
// Special treatment For sign-bit extraction patterns (lt comparison with | ||
// zero), always use signed comparison to preserve the semantic intent | ||
if (op.getKind() == cir::CmpOpKind::lt && isCIRZeroVector(op.getRhs())) { | ||
shouldUseSigned = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be forcing signedness for no good reason, like you previously stated in the description, we know this comes from unsigned. This is a fair IR difference to live with, the question is weather this is too aggressive for the canonicalizer to be doing or if we want to move this into CIR simplify. I think the current behavior is good enough. Can you instead add a C source test for both unsigned and signed versions and capture that the canonicalizer kicks for one and not for the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the detailed response. Just to confirm — in the context of this PR, we want to preserve the current lowering logic, and my next step would be to add tests documenting the current behavior. Is that correct?
We just went through a rebase, this PR needs to be updated. |
827f592
to
e040553
Compare
Updated this along with my other currently open PR's |
While working on _mm_movepi8_mask, intrinsic (and similar sign-bit checking intrinsics containing 8-bit integers) was being optimized away when using -fno-signed-char. Effectively replacing a cmp expression for 0.
Since unsigned values can never be less than zero, the CIR lowering was directly generating a constant 0 (I suppose we fold a vec filled with 0's to our target, which is a scalar mask, which in turn is 0) instead of the intended comparison operation, completely eliminating the icmp instruction.
See when passing as arg -fno-signed-char (no cmp generated):
OG:
CIR:
Since integer signedness is something we can track, the behaviour CIR is enforcing makes sense; however, if we want to preserve parity with OG, I believe this patch will match that. I can close this PR if that's not applicable to this case.
Added special case detection for sign-bit extraction patterns (lt comparison with cir::ZeroAttr) to force signed comparison regardless of the element type's signedness. This preserves the semantic intent of checking sign bits rather than performing mathematical unsigned comparisons.