-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add Ord::cmp
for primitives as a BinOp
in MIR
#118310
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ use crate::common::{self, IntPredicate}; | |
use crate::traits::*; | ||
use crate::MemFlags; | ||
|
||
use rustc_hir as hir; | ||
use rustc_middle::mir; | ||
use rustc_middle::mir::Operand; | ||
use rustc_middle::ty::cast::{CastTy, IntTy}; | ||
|
@@ -882,6 +883,35 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { | |
bx.icmp(base::bin_op_to_icmp_predicate(op.to_hir_binop(), is_signed), lhs, rhs) | ||
} | ||
} | ||
mir::BinOp::Cmp => { | ||
use std::cmp::Ordering; | ||
debug_assert!(!is_float); | ||
let pred = |op| base::bin_op_to_icmp_predicate(op, is_signed); | ||
if bx.cx().tcx().sess.opts.optimize == OptLevel::No { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The different approach makes quite a remarkable difference in unoptimized mode. I haven't investigated exactly what's happening in tablegen, but the unsigned_cmp:
cmp cx, dx
seta al
and al, 1
cmp cx, dx
setb cl
and cl, 1
sub al, cl
ret whereas the other approach gives unsigned_cmp:
push rax
mov word ptr [rsp + 2], dx
mov word ptr [rsp + 4], cx
mov al, 1
xor r8d, r8d
mov byte ptr [rsp + 6], r8b
cmp cx, dx
mov byte ptr [rsp + 7], al
jne .LBB1_2
mov al, byte ptr [rsp + 6]
mov byte ptr [rsp + 7], al
.LBB1_2:
mov cx, word ptr [rsp + 4]
mov dx, word ptr [rsp + 2]
mov al, byte ptr [rsp + 7]
mov byte ptr [rsp], al
mov al, 255
cmp cx, dx
mov byte ptr [rsp + 1], al
jb .LBB1_4
mov al, byte ptr [rsp]
mov byte ptr [rsp + 1], al
.LBB1_4:
mov al, byte ptr [rsp + 1]
pop rcx
ret |
||
// FIXME: This actually generates tighter assembly, and is a classic trick | ||
// <https://graphics.stanford.edu/~seander/bithacks.html#CopyIntegerSign> | ||
// However, as of 2023-11 it optimizes worse in things like derived | ||
// `PartialOrd`, so only use it in debug for now. Once LLVM can handle it | ||
// better (see <https://github.com/llvm/llvm-project/issues/73417>), it'll | ||
// be worth trying it in optimized builds as well. | ||
let is_gt = bx.icmp(pred(hir::BinOpKind::Gt), lhs, rhs); | ||
let gtext = bx.zext(is_gt, bx.type_i8()); | ||
let is_lt = bx.icmp(pred(hir::BinOpKind::Lt), lhs, rhs); | ||
let ltext = bx.zext(is_lt, bx.type_i8()); | ||
bx.unchecked_ssub(gtext, ltext) | ||
} else { | ||
// These operations are those expected by `tests/codegen/integer-cmp.rs`, | ||
// from <https://github.com/rust-lang/rust/pull/63767>. | ||
let is_lt = bx.icmp(pred(hir::BinOpKind::Lt), lhs, rhs); | ||
let is_ne = bx.icmp(pred(hir::BinOpKind::Ne), lhs, rhs); | ||
let ge = bx.select( | ||
is_ne, | ||
bx.cx().const_i8(Ordering::Greater as i8), | ||
bx.cx().const_i8(Ordering::Equal as i8), | ||
); | ||
bx.select(is_lt, bx.cx().const_i8(Ordering::Less as i8), ge) | ||
} | ||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1444,6 +1444,16 @@ pub enum BinOp { | |
Ge, | ||
/// The `>` operator (greater than) | ||
Gt, | ||
/// The `<=>` operator (three-way comparison, like `Ord::cmp`) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should say which values can be returned. I assume the return type is the same as the input type? But no that doesn't work when comparing unsigned integers. This files serves as our de-facto "MIR LangRef". EDIT: Oh wait this makes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ended up making it a primitive type because that was the easiest way to make the MIR validation happy. You're right that I should add more docs about that; will do. The alternative would be to have this always return EDIT: Updated the PR. |
||
/// | ||
/// This is supported only on the integer types and `char`, always returning | ||
/// [`rustc_hir::LangItem::OrderingEnum`] (aka [`std::cmp::Ordering`]). | ||
/// | ||
/// [`Rvalue::BinaryOp`]`(BinOp::Cmp, A, B)` returns | ||
/// - `Ordering::Less` (`-1_i8`, as a Scalar) if `A < B` | ||
/// - `Ordering::Equal` (`0_i8`, as a Scalar) if `A == B` | ||
/// - `Ordering::Greater` (`+1_i8`, as a Scalar) if `A > B` | ||
Cmp, | ||
/// The `ptr.offset` operator | ||
Offset, | ||
} | ||
|
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.
In the same file in Line 71,
BinOp::Cmp
was added to the same Branch as the other Compersions. As it doesn't make a difference here, the same could be done here.