Improved assumptions relating to isqrt#155265
Conversation
|
r? @jhpratt rustbot has assigned @jhpratt. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? tgross35 |
|
Could you include a godbolt link or before+after asm outputs indicating what all improves here? |
There was a problem hiding this comment.
Accidentally reviewed at the wrong PR, copied the comments over from #154115 (comment).
| crate::hint::assert_unchecked(result * result <= $n); | ||
| crate::hint::assert_unchecked(result <= $n); |
There was a problem hiding this comment.
Aren't these two redundant? If not, could you show an asm case indicating that?
There was a problem hiding this comment.
fn foo(x: u32) -> (u32, u32) {
let sqrt = smart_sqrt(x);
let a = x - sqrt;
let b = x - sqrt*sqrt;
(a, b)
}Overflow checks will be emitted when computing a regardless of crate::hint::assert_unchecked(result * result <= $n) and overflow checks will be emitted when computing b regardless of crate::hint::assert_unchecked(result <= $n) so both are necessary.
|
Reminder, once the PR becomes ready for a review, use |
|
@rustbot ready |
This comment has been minimized.
This comment has been minimized.
|
i have no idea how my computer missed the missing |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Improved assumptions relating to isqrt
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (c201a78): comparison URL. Overall result: no relevant changes - no action neededBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. @bors rollup=never Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (secondary 2.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.3%, secondary -3.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 490.276s -> 490.941s (0.14%) |
|
@bors r+ |
|
@bors r- |
|
This comment has been minimized.
This comment has been minimized.
|
@rustbot ready |
|
Since perf didn't seem to report anything, |
…uwer Rollup of 10 pull requests Successful merges: - #146544 (mir-opt: Remove the workaround in UnreachableEnumBranching) - #154819 (Fix ICE for inherent associated type mismatches) - #155265 (Improved assumptions relating to isqrt) - #152576 (c-variadic: use `emit_ptr_va_arg` for mips) - #154481 (Mark a function only used in nightly as nightly only) - #155614 (c-variadic: rename `VaList::arg` to `VaList::next_arg`) - #155630 (Make `//@ skip-filecheck` a normal compiletest directive) - #155641 (Remove non-working code for "running" mir-opt tests) - #155652 (Expand `Path::is_empty` docs) - #155656 (rustc_llvm: update opt-level handling for LLVM 23)
Rollup merge of #155265 - Apersoma:isqrt-smarter, r=jhpratt,tgross35 Improved assumptions relating to isqrt Improved various assumptions relating to values yielded by `isqrt`. Does not solve but does improve #132763. Re-openeing of #154115 Added assumptions are: * if `x` is nonzero then `x.isqrt()` is nonzero * `x.isqrt() <= x` * `x.isqrt() * x.isqrt() <= x`
…uwer Rollup of 10 pull requests Successful merges: - rust-lang/rust#146544 (mir-opt: Remove the workaround in UnreachableEnumBranching) - rust-lang/rust#154819 (Fix ICE for inherent associated type mismatches) - rust-lang/rust#155265 (Improved assumptions relating to isqrt) - rust-lang/rust#152576 (c-variadic: use `emit_ptr_va_arg` for mips) - rust-lang/rust#154481 (Mark a function only used in nightly as nightly only) - rust-lang/rust#155614 (c-variadic: rename `VaList::arg` to `VaList::next_arg`) - rust-lang/rust#155630 (Make `//@ skip-filecheck` a normal compiletest directive) - rust-lang/rust#155641 (Remove non-working code for "running" mir-opt tests) - rust-lang/rust#155652 (Expand `Path::is_empty` docs) - rust-lang/rust#155656 (rustc_llvm: update opt-level handling for LLVM 23)
…uwer Rollup of 10 pull requests Successful merges: - rust-lang/rust#146544 (mir-opt: Remove the workaround in UnreachableEnumBranching) - rust-lang/rust#154819 (Fix ICE for inherent associated type mismatches) - rust-lang/rust#155265 (Improved assumptions relating to isqrt) - rust-lang/rust#152576 (c-variadic: use `emit_ptr_va_arg` for mips) - rust-lang/rust#154481 (Mark a function only used in nightly as nightly only) - rust-lang/rust#155614 (c-variadic: rename `VaList::arg` to `VaList::next_arg`) - rust-lang/rust#155630 (Make `//@ skip-filecheck` a normal compiletest directive) - rust-lang/rust#155641 (Remove non-working code for "running" mir-opt tests) - rust-lang/rust#155652 (Expand `Path::is_empty` docs) - rust-lang/rust#155656 (rustc_llvm: update opt-level handling for LLVM 23)
View all comments
Improved various assumptions relating to values yielded by
isqrt.Does not solve but does improve #132763.
Re-openeing of #154115
Added assumptions are:
xis nonzero thenx.isqrt()is nonzerox.isqrt() <= xx.isqrt() * x.isqrt() <= x