jbe
March 25, 2022, 5:21pm
1
Is it alright to assume that the following assertions always hold? I assume the internal representation of std::cmp::Ordering
is an implementation detail that could change any time?
fn main() {
assert!((7.cmp(&9) as i32) < 0);
assert_eq!(7.cmp(&9) as i32, -1);
}
(Playground )
I don't think so.
Looking at the definition...
#[stable(feature = "rust1", since = "1.0.0")]
#[repr(i8)]
pub enum Ordering {
/// An ordering where a compared value is less than another.
#[stable(feature = "rust1", since = "1.0.0")]
Less = -1,
/// An ordering where a compared value is equal to another.
#[stable(feature = "rust1", since = "1.0.0")]
Equal = 0,
/// An ordering where a compared value is greater than another.
#[stable(feature = "rust1", since = "1.0.0")]
Greater = 1,
}
... The enum has a #[repr(i8)]
attribute and the overall type is stable.
That means you can rely on Ordering::Less
being -1
, Ordering::Equal
being 0
, and Ordering::Greater
being 1
. Furthermore, the i8
representation means the behaviour when casting to i32
or other types is also well-specified and not going to change.
The as i32
behaviour is visible to safe code on stable, so I think you're fine to assume it'll stay that way.
I wouldn't write any code that depends on the repr(i8)
, though. repr
s aren't necessarily part of the public API, the same way that size_of
isn't necessarily stable.
That said, why not just do
assert_eq!(7.cmp(&9), Ordering::Less);
instead?
The release notes for 1.57 include the PR that added the repr, and comments on the PR seem to indicate this was intended to be a guaranteed, user-visible property.
1 Like
jbe
March 25, 2022, 5:36pm
5
Michael-F-Bryan:
I don't think so.
I was worried because rustdoc didn't show the values for the enum variants.
Does stable
refer to the particular integer representation as well as the name of the enum variant?
That would mean,
#[stable(feature = "rust1", since = "1.0.0")]
#[repr(i8)]
pub enum Ordering {
Here, stable
refers to enum Ordering
but not to the repr
annotation?
scottmcm:
That said, why not just do
assert_eq!(7.cmp(&9), Ordering::Less);
instead?
My use case is the following:
unsafe extern "C" fn compare_function<T>(a: *const lmdb::MDB_val, b: *const lmdb::MDB_val) -> c_int
where
T: ?Sized + Storable,
{
let a: &[u8] =
slice::from_raw_parts((*a).mv_data as *const u8, (*a).mv_size.try_into().unwrap());
let b: &[u8] =
slice::from_raw_parts((*b).mv_data as *const u8, (*b).mv_size.try_into().unwrap());
match T::cmp_bytes_unchecked(a, b) {
Ordering::Less => -1,
Ordering::Equal => 0,
Ordering::Greater => 1,
}
}
That's surprising. If they don't want people to make any assumptions about the type's layout, you would think they'd leave it as repr(Rust)
and not provide any values for each enum variant
1 Like
jbe:
match T::cmp_bytes_unchecked(a, b) {
Ordering::Less => -1,
Ordering::Equal => 0,
Ordering::Greater => 1,
}
Well, the good news is that the repr
is irrelevant to replacing that was as
, so you're fine.
I was initially going to say that you should just leave the match and let LLVM deal with it, but it looks like LLVM doesn't notice that it's identity today. I filed Identity `match` on field-less enum compiles worse than using `as` · Issue #95313 · rust-lang/rust · GitHub about that.
But if you're really worried about assembly-peeking, you might want to add a cmp_bytes_unchecked_raw
that returns a c_int
, since normalizing to -1, 0, 1
for the enum adds some cost, compared to just returning the c_int
from the memcmp
I assume it's using internally.
This one definitely doesn't follow, because as i32
still works without the repr
, so they might add the custom discriminants to enable that without intending to say anything about the layout.
It depends if they want to use it as an implementation detail for unsafe
code elsewhere in the library.
See, for example, how it's still an FCP to say "it has the same layout as T
" despite the repr(transparent)
in the implementation: Clarify that ManuallyDrop<T> has same layout as T by joshlf · Pull Request #88375 · rust-lang/rust · GitHub
1 Like
jbe
March 25, 2022, 6:18pm
8
I was hoping the match
is zero-cost, but apparently it's not (yet).
scottmcm:
But if you're really worried about assembly-peeking, you might want to add a cmp_bytes_unchecked_raw
that returns a c_int
, since normalizing to -1, 0, 1
for the enum adds some cost, compared to just returning the c_int
from the memcmp
I assume it's using internally.
Well, my default implementation uses std::cmp::Ord::cmp
, so the return type of my implementation will be std::cmp::Ordering
:
/// Types that can be stored
pub unsafe trait Storable: Ord {
/* … */
/// Pointer to aligned version of Self
type AlignedRef<'a>: Deref<Target = Self> + PointerIntoOwned;
/// Pointer to byte representation
type BytesRef<'a>: Deref<Target = [u8]>
where
Self: 'a;
/// Converts to byte slice
fn to_bytes(&self) -> Self::BytesRef<'_>;
/// Converts from byte slice
unsafe fn from_bytes_unchecked(bytes: &[u8]) -> Self::AlignedRef<'_>;
/// Compares byte representation
unsafe fn cmp_bytes_unchecked(a: &[u8], b: &[u8]) -> Ordering {
Self::from_bytes_unchecked(a).cmp(&Self::from_bytes_unchecked(b))
}
}
Maybe it makes sense to add a corresponding note to std::cmp::Ordering
's documentation, so the intention gets clear?
1 Like
Definitely. It's always good to send a PR adding the guarantee to the docs explicitly, and the libs-api can FCP a decision to say "yes, we'll guarantee that". Even for behaviour visible in safe code, like Document the order of {Vec,VecDeque,String}::retain by cuviper · Pull Request #60396 · rust-lang/rust · GitHub
I consider a repr
to be part of the public API of structs if not documented otherwise. It often has to be, for repr(C)
and repr(transparent)
, say.
Update on the missing optimization: C++ has the same problem (https://clang.godbolt.org/z/57jqWxbxe ), so it's not rust's fault, and thus here's the bug on LLVM if you're curious to follow along
opened 06:35PM - 25 Mar 22 UTC
llvm:optimizations
Opt transformation showing suboptimal current result on trunk: <https://llvm.god… bolt.org/z/jj6vcv5q3>
Alive2 demonstration that the proposed transformation is correct: <https://alive2.llvm.org/ce/z/hAAoh_>
Original motivating example in https://github.com/rust-lang/rust/issues/95313: <https://rust.godbolt.org/z/dvYe9r78c>
The following code:
```llvm
define i32 @src(i8 noundef %0) {
start:
%1 = alloca i32, align 4
%order = alloca i8, align 1
store i8 %0, i8* %order, align 1
%_2 = load i8, i8* %order, align 1, !range !2, !noundef !3
switch i8 %_2, label %bb2 [
i8 -1, label %bb3
i8 0, label %bb4
i8 1, label %bb1
]
bb2: ; preds = %start
unreachable
bb3: ; preds = %start
store i32 -1, i32* %1, align 4
br label %bb5
bb4: ; preds = %start
store i32 0, i32* %1, align 4
br label %bb5
bb1: ; preds = %start
store i32 1, i32* %1, align 4
br label %bb5
bb5: ; preds = %bb3, %bb4, %bb1
%2 = load i32, i32* %1, align 4
ret i32 %2
}
!2 = !{i8 -1, i8 2}
!3 = !{}
```
Optimizes to this slightly suboptimal IR:
```llvm
define i32 @src(i8 noundef %0) local_unnamed_addr #0 {
%switch.tableidx = add i8 %0, 1
%switch.idx.cast = zext i8 %switch.tableidx to i32
%switch.offset = add nsw i32 %switch.idx.cast, -1
ret i32 %switch.offset
}
```
But as Alive confirms, it could just be a `sext`:
```llvm
define i32 @tgt(i8 noundef %0) {
%start:
%1 = sext i8 noundef %0 to i32
ret i32 %1
}
// Transformation seems to be correct!
```
1 Like
system
Closed
June 23, 2022, 11:21pm
12
This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.