Use wrapping arithmetic in from_str_radix - #163099
Conversation
|
@rust-timer queue |
This comment has been minimized.
This comment has been minimized.
|
You need to run bors ( |
|
@bors try |
This comment has been minimized.
This comment has been minimized.
[EXPERIMENT] Use unchecked arithmetic in `from_str_radix`
|
Is rustbot broken? @rustbot label T-libs |
|
Error: Label waiting-on-author can only be set by Rust team members Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip. |
|
The waiting-on-author label has an S- prefix |
|
Alright so godbolt reveals that they generate the same optimized IR with overflow checks (for i.e. this should not change runtime perf when running without overflow checks, but with overflow checks it should cause a decent amount of improvement (and get rid of the panics) |
This comment has been minimized.
This comment has been minimized.
eca2552 to
2e0c4ed
Compare
|
Since this makes the correctness of |
|
Finished benchmarking commit (1d6776a): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -2.4%, secondary 2.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.0%, secondary -4.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%, secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 499.144s -> 497.776s (-0.27%) |
from_str_radixfrom_str_radix
|
r? libs edit: lol |
|
@rustbot label I-libs-nominated |
2e0c4ed to
75f50bf
Compare
|
If this produces the same assembly for |
|
Agreed. I'll add a comment mentioning this though |
from_str_radixfrom_str_radix
75f50bf to
3e43dc9
Compare
3e43dc9 to
84a341b
Compare
|
@rustbot ready Actually handing review back to Alice |
…d, r=Darksonn Use wrapping arithmetic in `from_str_radix` Uses wrapping arithmetic in the fast/unchecked loop of `from_str_radix`. Originally this PR was written using unchecked arithmetic: This generates identical assembly when compiling with `-O`: https://godbolt.org/z/Gvb7G9YEM However, it significantly simplifies the asm of `-O -Coverflow-checks` (e.g. RfL): https://godbolt.org/z/Pah9fMfrd More precisely, the `-O -Coverflow-checks` output becomes identical to the `-O` one: https://godbolt.org/z/WoYMThP8e In conclusion, it makes more sense to use wrapping operations here.
There was a problem hiding this comment.
If the goal is to actually remove panics and keep them gone then why no codegen test?
There was a problem hiding this comment.
The function can already panic from the radix check. This change just brings the loop in line with the no-overflow-check version, which is the default. Adding a test feels more like an obligation that we will keep it this way. Idk if that is what we want, but I'm open to discussion on zulip ^^
…uwer Rollup of 13 pull requests Successful merges: - #156949 (Detect missing else in let statement) - #160436 (stabilize `Box::take`) - #160570 (macro_metavar_expr_concat: support concatenating into string literals) - #162837 (Dedicated Display type for CStr::display) - #163099 (Use wrapping arithmetic in `from_str_radix`) - #163166 (Tiny cleanups to deferred liveness) - #161667 (Add `f16` inline ASM support for `nvptx64-nvidia-cuda`) - #163063 (Restore `Send` and `Sync` for `BorrowedCursor`) - #163097 (OpenBSD/sparc64 has switched from GCC to Clang) - #163126 (Skip redundant storage-conflict updates during coroutine layout) - #163135 (librustdoc: remove stale dep on base64) - #163146 (tests: Update `f16b` codegen test for LoongArch and RISC-V) - #163159 (treat inductive cycles as ambig)
Rollup merge of #163099 - maxdexh:int-from-str-loop-unchecked, r=Darksonn Use wrapping arithmetic in `from_str_radix` Uses wrapping arithmetic in the fast/unchecked loop of `from_str_radix`. Originally this PR was written using unchecked arithmetic: This generates identical assembly when compiling with `-O`: https://godbolt.org/z/Gvb7G9YEM However, it significantly simplifies the asm of `-O -Coverflow-checks` (e.g. RfL): https://godbolt.org/z/Pah9fMfrd More precisely, the `-O -Coverflow-checks` output becomes identical to the `-O` one: https://godbolt.org/z/WoYMThP8e In conclusion, it makes more sense to use wrapping operations here.
…d, r=Darksonn Use wrapping arithmetic in `from_str_radix` Uses wrapping arithmetic in the fast/unchecked loop of `from_str_radix`. Originally this PR was written using unchecked arithmetic: This generates identical assembly when compiling with `-O`: https://godbolt.org/z/Gvb7G9YEM However, it significantly simplifies the asm of `-O -Coverflow-checks` (e.g. RfL): https://godbolt.org/z/Pah9fMfrd More precisely, the `-O -Coverflow-checks` output becomes identical to the `-O` one: https://godbolt.org/z/WoYMThP8e In conclusion, it makes more sense to use wrapping operations here.
View all comments
Uses wrapping arithmetic in the fast/unchecked loop of
from_str_radix.Originally this PR was written using unchecked arithmetic:
This generates identical assembly when compiling with
-O: https://godbolt.org/z/Gvb7G9YEMHowever, it significantly simplifies the asm of
-O -Coverflow-checks(e.g. RfL): https://godbolt.org/z/Pah9fMfrdMore precisely, the
-O -Coverflow-checksoutput becomes identical to the-Oone: https://godbolt.org/z/WoYMThP8eIn conclusion, it makes more sense to use wrapping operations here.