Use unchecked mul to compute slice sizes #98078
Conversation
rustbot
added
the
T-compiler
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
| @@ -39,7 +39,8 @@ pub fn size_and_align_of_dst<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( | |||
| // The info in this case is the length of the str, so the size is that | |||
| // times the unit size. | |||
| ( | |||
| bx.mul(info.unwrap(), bx.const_usize(unit.size.bytes())), | |||
| // All slice sizes must fit into `isize`, so this multiplication cannot (signed) wrap. | |||
| bx.unchecked_smul(info.unwrap(), bx.const_usize(unit.size.bytes())), | |||
There was a problem hiding this comment.
Any specific reason to use signed multiplication here?
Both length and element size are unsigned.
There was a problem hiding this comment.
Because mul/unchecked_smul/unchecked_umul are all "half width" multiplies (the result is the same number of bits as the input), there is no behavior difference between signed and unsigned multiplication--for the same bitpattern in the inputs, they return the same result. This is why the previous code was just bx.mul, and not bx.smul or bx.umul (which don't exist).
The difference between unchecked_umul/unchecked_smul, then, is that unchecked_umul tells the backend that the multiplication can't wrap around the unsigned boundary (0/usize::MAX), and unchecked_smul tells the backend that it can't wrap around the signed boundary (isize::MIN/isize::MAX). Ideally, we would have both (resulting in mul nsw nuw in LLVM IR), since we know both inputs are non-negative. But there's no existing unchecked_nsw_nuw_mul, and it's not necessary for this use case, so I don't think it's worth adding.
Because both unchecked_umul and unchecked_smul are correct here, the choice is arbitrary. I picked unchecked_smul here because the upper bound on the result is lower (isize::MAX instead of usize::MAX), though I have no particular reason to prefer that over a tighter lower bound. The added test case passes with either.
Added a comment.
|
r=me after answering #98078 (comment) and squashing commits. |
rustbot
added
S-waiting-on-author
...since slice sizes can't signed wrap see https://doc.rust-lang.org/std/slice/fn.from_raw_parts.html > The total size len * mem::size_of::<T>() of the slice must be no larger than isize::MAX.
erikdesjardins
force-pushed
the
uncheckedsize
branch
from
d7964d4
to
50f6a9e
Compare
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
Thanks! |
|
📌 Commit 50f6a9e has been approved by |
bors
added
S-waiting-on-bors
…rochenkov Use unchecked mul to compute slice sizes This allows LLVM to realize that `slice.len() > 0` iff `slice.len() * size_of::<T>() > 0`, allowing a branch on the latter to be folded into the former when dropping vecs and boxed slices, in some cases. Fixes (partially) rust-lang#96497
Rollup of 7 pull requests Successful merges: - rust-lang#97822 (Filter out intrinsics if we have other import candidates to suggest) - rust-lang#98026 (Move some tests to more reasonable directories) - rust-lang#98067 (compiler: remove unused deps) - rust-lang#98078 (Use unchecked mul to compute slice sizes) - rust-lang#98083 (Rename rustc_serialize::opaque::Encoder as MemEncoder.) - rust-lang#98087 (Suggest adding a `#[macro_export]` to a private macro) - rust-lang#98113 (Fix misspelling of "constraint" as "contraint") Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
This allows LLVM to realize that
slice.len() > 0iffslice.len() * size_of::<T>() > 0, allowing a branch on the latter to be folded into the former when dropping vecs and boxed slices, in some cases.Fixes (partially) #96497