-
Notifications
You must be signed in to change notification settings - Fork 14k
use funnel shift as fallback impl for rotating shifts #148478
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
Conversation
|
cc @rust-lang/wg-const-eval Some changes occurred to the CTFE machinery
cc @rust-lang/miri Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri |
|
rustbot has assigned @jdonszelmann. Use |
4824240 to
e47ba14
Compare
This comment has been minimized.
This comment has been minimized.
c27f09a to
a180c65
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
a180c65 to
fb03e19
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| #[must_use = "this returns the result of the operation, \ | ||
| without modifying the original"] | ||
| #[inline(always)] | ||
| #[rustc_allow_const_fn_unstable(const_trait_impl)] // for the intrinsic fallback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems fine to me. Absolutely worst case we can always remove bounds from the intrinsic and have the compiler do all the work again, but with the setup we have now for libstd bootstrap I'm not worried about even major changes anymore
|
@antoyo @bjorn3 in case you'd like to drop the rotate_left/right intrinsic implementations from your backends, that's possible now as there is a library fallback implementation in terms of the funnel shift intrinsics (which in turn also have a fallback implementation). However, at least cranelift seems to have native support for rotations so we probably want to keep the direct implementation in the backend? OTOH if we can remove it from all backends we can stop making this an intrinsic so that would also be nice. ;) |
af730a5 to
6ce020d
Compare
|
@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.
use funnel shift as fallback impl for rotating shifts
Indeed.
I haven't implemented funnel shift support yet and Cranelift doesn't have native support for this. |
This comment has been minimized.
This comment has been minimized.
6ce020d to
e347990
Compare
ec66871 to
a00db66
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors rollup- The "perf regression" is noise (a change in macro expansion for one benchmark) |
|
@jdonszelmann would you prefer me to reroll the reviewer here? |
|
I'm so sorry Ralf, didn't get around to it. r? compiler |
|
No worries. :) |
|
@bors r+ rollup=never |
|
Scheduling: Fall back to rollup=never if a rollup-of-iffy fails. @bors p=1 |
…imulacrum use funnel shift as fallback impl for rotating shifts That lets us remove this gnarly implementation from Miri and const-eval. However, `rotate_left`/`rotate_right` are stable as const fn, so to do this we have to `rustc_allow_const_fn_unstable` a bunch of const trait stuff. Is that a bad idea? Cc `@oli-obk` `@fee1-dead`
|
☀️ Test successful - checks-actions |
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 69d4d5f (parent) -> 89fe961 (this PR) Test differencesShow 6438 test diffs6438 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 89fe96197d232f86d733566df31c6dcebd1750da --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 (89fe961): comparison URL. Overall result: ❌✅ regressions and improvements - 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 0.3%, secondary 2.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 1.2%, secondary 0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 475.558s -> 476.458s (0.19%) |
|
Ah, that's not quite expected... if I understand the detailed results correctly, the eza regression is just the extra time spent generating the code for the fallback implementation? But the odd thing is, we did a perf run before and nothing relevant showed up. |
That lets us remove this gnarly implementation from Miri and const-eval.
However,
rotate_left/rotate_rightare stable as const fn, so to do this we have torustc_allow_const_fn_unstablea bunch of const trait stuff. Is that a bad idea? Cc @oli-obk @fee1-dead