Skip to content
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

Implement BitAnd-based truncation for integers #54442

Closed
wants to merge 2 commits into from

Conversation

npmccallum
Copy link
Contributor

There are a variety of circumstances where a function wants to take n
low bits from a generic input type. These implementations support that
workflow.

Closes #54435

There are a variety of circumstances where a function wants to take n
low bits from a generic input type. These implementations support that
workflow.

Closes rust-lang#54435
@rust-highfive
Copy link
Collaborator

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 21, 2018
@petrochenkov
Copy link
Contributor

petrochenkov commented Sep 21, 2018

Cross-type arithmetic/bitwise operations are generally not implemented for primitive types.
(See e.g. https://doc.rust-lang.org/std/primitive.u32.html#implementations, shifts are exceptions because their operands mean different things.)

Very often such type mismatch means an accidental mistake, and in that case the code author needs to harmonize operand types rather than get truncation performed implicitly.
If these impls are added, then we lose ability to detect such mistakes.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:06:24]    Compiling chalk-engine v0.7.0
[00:06:24]    Compiling env_logger v0.5.12
[00:06:25]    Compiling rustc_apfloat v0.0.0 (file:///checkout/src/librustc_apfloat)
[00:06:25]    Compiling parking_lot_core v0.3.0
[00:06:26] error[E0277]: no implementation for `u128 & i32`
[00:06:26]     --> librustc_apfloat/ieee.rs:2608:58
[00:06:26]      |
[00:06:26] 2608 |         let select = |limb, i| (limb >> (i * HALF_BITS)) & ((1 << HALF_BITS) - 1);
[00:06:26]      |                                                          ^ no implementation for `u128 & i32`
[00:06:26]      |
[00:06:26]      = help: the trait `std::ops::BitAnd<i32>` is not implemented for `u128`
[00:06:26] error: aborting due to previous error
[00:06:26] 
[00:06:26] For more information about this error, try `rustc --explain E0277`.
[00:06:26] error: Could not compile `rustc_apfloat`.
[00:06:26] error: Could not compile `rustc_apfloat`.
[00:06:26] warning: build failed, waiting for other jobs to finish...
[00:06:28] error: build failed
[00:06:28] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" " jemalloc" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:06:28] expected success, got: exit code: 101
[00:06:28] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1135:9
[00:06:28] travis_fold:end:stage0-rustc

[00:06:28] travis_time:end:stage0-rustc:start=1537550638622924466,finish=1537550666003542093,duration=27380617627


[00:06:28] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:06:28] Build completed unsuccessfully in 0:01:21
[00:06:28] Makefile:28: recipe for target 'all' failed
[00:06:28] make: *** [all] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1135272c
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
The command "date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
" exited with 0.
travis_fold:start:after_failure.1
travis_time:start:1350a1e0
$ echo "#### Build failed; Di ./obj/build/x86_64-unknown-linux-gnu/native/jemalloc
64960 ./src/test
62448 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc
60840 ./src/llvm-emscripten/lib
56436 ./src/llvm/test/MC

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@leonardo-m
Copy link

I agree on keeping cross-type arithmetic/bitwise operations out of Rust, yet "as" casts could be slowly phased out and replaced by more specialized and generally more reliable conversions. So adding some specialized methods (distinct from the operators) to perform the truncations could be OK.

@nagisa
Copy link
Member

nagisa commented Sep 21, 2018

This will make type interference not work right in some cases.

Currently 1234 & (32 : T) will infer 1234 to have type T as well because there is only one applicable trait that satisfies T on RHS. After a PR like this gets merged, 1234 would get inferred to i32.

@scottmcm
Copy link
Member

Smushing a few of the comments here together, maybe it would make sense to have explicitly-asymmetrical methods for some of these operations, to make the intent and asymmetrical type handling clearer?

@nagisa
Copy link
Member

nagisa commented Sep 22, 2018

An explicit method would be dope. Furthermore, it doesn’t need to be a part of libstd proper and can be published as a crate instead.

@npmccallum
Copy link
Contributor Author

So, we already have CastInto<T>. Can't we stabilize that?

@npmccallum
Copy link
Contributor Author

@nagisa I disagree strongly. Rust's number system is so painful to use in generic contexts that we literally have a whole set of crates (rust-num*) just to try to make it workable. I completely agree that libstd should be kept small. But the types it provide need to be usable without requiring external dependencies.

The current situation is that conversion between the integer types is common, but painful. Further, a good number of the useful methods/functions aren't in traits, so they can't be specified in a generic binding.

@leonardo-m
Copy link

leonardo-m commented Sep 25, 2018

The current situation is that conversion between the integer types is common, but painful.

This mostly true, but on the other hand languages that allow cross-type arithmetic/bitwise operations lead very quickly to programs that are soup of types. I see this very well when I convert C/C++/Java code to Rust. Most of that conversion pain is caused by the very wild typing of the original code and not by Rust strictness or by needs of the algorithms or by performance needs. This is another case where Rust strictness leads to (much) better code.

@npmccallum
Copy link
Contributor Author

@leonardo-m I agree. But this is solvable with the right traits and compiler support. The reason I list both is that we don't want a situation where common operations in a generic setting carry a performance penalty. For example, "cast to u8 and bitwise and" should emit a single instruction, not a function call.

Here's what I see as needed improvements:

  1. Stabilize CastInto<T> so that we can get zero runtime cost casting behind generics.

  2. Make it possible to have traits with const associated functions. For example:

trait Bounded: Sized {
  const fn max_value() -> Self;
  const fn min_value() -> Self;
}

impl Bounded for i8 { ... }
...
impl Bounded for u128 { ... }
  1. Trait-ify common integer associated functions and methods. For example, checked_abs(). Require that new functions on integers be applied as traits.

  2. Merge a significant amount of rust-num-traits into libstd.

  3. Create associated type relations between the signed and unsigned variants. Something like:

trait SizeEquivalence {
  type Signed;
  type Unsigned;
}

impl SizeEquivalence for u8 {
  type Signed = i8;
  type Unsigned = u8;
}
...
  1. Make a way to generically create integer types from an integer literal. This should have as little runtime cost as possible. For example:
let x: Self = 0;

The above code should emit zero instructions on architectures with a zero register and one instruction otherwise.

@npmccallum
Copy link
Contributor Author

Also, the TryFrom<T> conversions are great. But the associated run time penalty of using them makes them much less valuable.

@sfackler
Copy link
Member

For example, "cast to u8 and bitwise and" should emit a single instruction, not a function call.

Why would it emit a function call? Inlining is performed extensively on Rust code for exactly this kind of situation.

@npmccallum
Copy link
Contributor Author

Also, trait exclusion...

trait Signed: !Unsigned {}
trait Unsigned: !Signed {}

@npmccallum
Copy link
Contributor Author

@sfackler My full comment was clear I meant in a generic context. Currently, there is no way to perform a cast behind a generic. We will get closer when TryFrom<T> stabilizes, but his will certainly emit more than zero instructions.

@npmccallum
Copy link
Contributor Author

One example of what we should be able to write is this:

fn low7<T: Copy + CastInto<u8>>(value: T) -> u8 {
  value as u8 & 0b01111111
}

@sfackler
Copy link
Member

but his will certainly emit more than zero instructions.

Why?

@npmccallum
Copy link
Contributor Author

@sfackler Run time error handling because TryFrom<T> is not infallible.

@sfackler
Copy link
Member

Uh, sure, different operations have different performance characteristics. You can write low7 right now - the only thing you need to do is value.cast() (or whatever CastInto's method is named) rather than value as u8.

@npmccallum
Copy link
Contributor Author

@sfackler Not on stable. Not ever according to the docs.

@sfackler
Copy link
Member

pub trait CastInto<T> {
    fn cast_into(v: Self) -> T;
}

impl CastInto<u8> for u32 {
    #[inline]
    fn cast_into(v: u32) -> u8 {
        v as u32
    }
}

a.cast_into() will be literally the same thing as a as u8.

@npmccallum
Copy link
Contributor Author

@sfackler I'm not questioning that. I'm suggesting that Rust consider stabilizing CastInto<T>.

@npmccallum
Copy link
Contributor Author

Maybe we should implement two traits: one for truncated conversion and one for size-equivalence conversion. Thus, u128 can be truncated to u8. However, truncated conversion is not implemented for u8 to u128. Further, truncated conversion is not implemented for u128 to i8, since this would cross the signedness barrier. The latter could be explicitly handled by the size-equivalence conversion.

For example, to convert a u128 integer to an i8:

127u128.signed().truncate::<i8>()

@sfackler
Copy link
Member

A third party crate seems like a good place to design an API like that.

@scottmcm
Copy link
Member

.cast_into() appears to be exactly the same as .wrapping_into() from rust-lang/rfcs#2484

@TimNN
Copy link
Contributor

TimNN commented Oct 16, 2018

Ping from triage! It seem to me that this PR is a somewhat fundamental change requiring an RFC or at least some additional discussion. As such I'm closing this as blocked for now. Please comment / re-open if you disagree. Thanks for you contribution!

@TimNN TimNN closed this Oct 16, 2018
@TimNN TimNN added S-blocked-closed and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 16, 2018
@jyn514 jyn514 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-blocked-closed labels Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using BitAnd<T> for truncation on integer types?
9 participants