constify Step trait and all of its implementations#153821
constify Step trait and all of its implementations#153821rust-bors[bot] merged 1 commit intorust-lang:mainfrom
Step trait and all of its implementations#153821Conversation
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
Step trait and all of its implementations
Step trait and all of its implementationsStep trait and all of its implementations
| // CHECK: %[[B01:.+]] = load i16, ptr %[[PB01]] | ||
| // CHECK-NOT: cmp | ||
| // CHECK: %[[EQ01:.+]] = icmp eq i16 %[[A01]], %[[B01]] | ||
| // CHECK-NEXT: br i1 %[[EQ01]], label %{{.+}}, label %[[EXIT_U]] |
There was a problem hiding this comment.
Do we know what happened to cause this? Maybe one of the const-hack closure -> function conversions above changed inlining? If I'm reading this right, the new IR seems worse -- 0/2/4/6 vs. 0/4/6/2 seems like it might be at least a tiny regression.
There was a problem hiding this comment.
cc @scottmcm in case you have thoughts on the new IR -- I think it meets the goal of when we added it still...
There was a problem hiding this comment.
my guess was this swapped basically by chance and since the rest is the same it should be fine.
There was a problem hiding this comment.
It's definitely weird that it swapped things like this in such a way that the only branch to the first block is from below -- or at least I'm assuming that that's why it ended up without a header? Should probably try to fix that so the block name substitutions are still meaningful?
But in general, I'm not worried about the block order of the things, since that's really LLVM's business not ours, and is highly divorced from what we can do in code anyway.
That said, I'm definitely sad about all these const hacks, and kinda wish they'd stop until the features existed -- if it needs const closures it should use const closures, for example. But that's not really my call :/
There was a problem hiding this comment.
Const closures were reimplemented very recently and to my knowledge aren't supposed to be used in the std lib yet, these are rather tame const hacks.
| let rhs = &right[..l]; | ||
|
|
||
| for i in 0..l { | ||
| // FIXME(const-hack): revert this to `for i in 0..l` once `impl const Iterator for Range<T>` |
There was a problem hiding this comment.
Definitely sad that this PR fixing Step that's the main thing this needs still can't just use in 0..l 😢
There was a problem hiding this comment.
Kind of a chicken and egg problem here, this is pretty much the last thing needed for const Ranges.
|
@bors r+ rollup=never |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 79531c5 (parent) -> 7e28c74 (this PR) Test differencesShow 332 test diffs332 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 7e28c7438a7b0c79a09724ba799d32ed461beb72 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (7e28c74): comparison URL. Overall result: ❌ regressions - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +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.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResults (primary 1.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 484.626s -> 484.417s (-0.04%) |
|
Looks like a tiny regression in debug and opt benchmarks. Not sure why would messing with |
|
It might be the const hacks causing slightly different inlining or something along those lines? I think it's probably not worth trying to dig into it given just one regression. |
|
Ok, agreed. @rustbot label: +perf-regression-triaged |
… r=jhpratt constify `Step for NonZero<u*>` Tracking Issue: rust-lang#42168 I missed the constification of `Step for NonZero<u*>` in the recent rust-lang#153821, so here they are. Had to constify the `Clone` / `TrivialClone` impls along the way rust-lang#142757 .
… r=jhpratt constify `Step for NonZero<u*>` Tracking Issue: rust-lang#42168 I missed the constification of `Step for NonZero<u*>` in the recent rust-lang#153821, so here they are. Had to constify the `Clone` / `TrivialClone` impls along the way rust-lang#142757 .
constify `Step for NonZero<u*>` Tracking Issue: rust-lang/rust#42168 I missed the constification of `Step for NonZero<u*>` in the recent rust-lang/rust#153821, so here they are. Had to constify the `Clone` / `TrivialClone` impls along the way rust-lang/rust#142757 .
constifying Step trait and all of its implementations, with some friendly help from const_cmp