Repository navigation
fix(mir-lower): fall back to live operand type in arithmetic signedness check - #141
Merged
nihalpasham merged 7 commits intoJun 12, 2026
Merged
Conversation
…ss check
`is_signed_int_op` errored ("expected IntegerType or MirPtrType operand") when the
pre-conversion MIR type was not tracked for an operand — which happens for booleans
and values produced inside short-circuit `||`/`&&`, or already-converted operands.
Fall back to the live operand type: a (signless, post-conversion) integer or a
pointer is treated as unsigned, instead of failing the whole lowering.
nihalpasham
added
bug
The fallback added for untracked operands treated any live integer as unsigned. That is correct today because the only values without conversion history are signless i1 bools (signless is not Signed), but a blanket "unsigned" would silently miscompile if a future pass ever produced an untracked Signed-typed operand. Consult the live type's signedness instead, and correct the comment: replace_operation records history for already-converted operands, so the untracked case is exactly values whose type was never convertible.
Builds the short-circuit shape (let p = a || b; p == q; p < q) directly in dialect-mir: a merge-block i1 argument has no conversion history because signless i1 is rejected by can_convert_type, so the comparison must go through the live-type fallback in is_signed_int_op. Asserts the lowered body contains exactly icmp eq i1 and icmp ult i1. Reverting the fallback reproduces the original failure (expected IntegerType or MirPtrType operand in arithmetic op).
End-to-end cover for PR NVIDIA#141 through the full device pipeline: a bool phi produced by short-circuit || feeds both == and < comparisons, and the lanes cover all four (p, q) truth combinations. Before the fallback in is_signed_int_op this kernel failed device codegen with "expected IntegerType or MirPtrType operand in arithmetic op"; the emitted IR now contains icmp eq i1 and icmp ult i1 over the phi.
# Conflicts: # crates/mir-lower/tests/lowering_test.rs
…i_cmp clippy::bool_comparison wants p < q rewritten as !p & q, but the comparison shape is the regression under test: it must reach mir-lower as an order comparison on an untracked i1 phi and lower to icmp ult i1.
Collaborator
|
This LGTM 👍🏾 Added three commits layered on top:
One correction folded into the first commit: "already-converted operands" in the comment was not quite right, since |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
is_signed_int_operrored ("expected IntegerType or MirPtrType operand") when the pre-conversion MIR type was not tracked for an operand — which happens for booleans and values produced inside short-circuit||/&&, or already-converted operands.Fall back to the live operand type: a (signless, post-conversion) integer or a pointer is treated as unsigned, instead of failing the whole lowering.