-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Meta tracking issue for branch hints (RFC 1131) #26179
Comments
FWIW, looking through my own code, I actually think the fact that this can't apply to match variants is going to be fairly hard on idiomatic Rust (for example, it means you can't use it when matching on an |
@pythonesque once I get this done, I plan on writing a follow-up RFC for a |
It might be worth having an "unpredictable" option in addition to likely and unlikely. There is now something to lower it to in LLVM: http://reviews.llvm.org/D12341 |
@Aatch Are you still working on this? |
Shall
I expect for most code Ok is likely and Err is unlikely. Of course, annotating |
@vi I think hinting should be only be used rarely, so it being on the type doesn't seem like a good idea. |
@Aatch, Is it a good idea to have |
@vi is is not. As a general rule, people are pretty bad at knowing what cases are actually likely or not. Branch hinting is a fairly specific optimisation hint, used incorrectly it can actually make code slower. It should also only really be used when the unlikely case is known to be very unlikely. Basically, you should only use it if it makes your code faster, otherwise it should be there. |
@vi It might be good idea if the likely thing is cheap, and the unlikely thing is expensive. For example, In my opinion, frequent use of |
Instead of arguing about it, why not just test it? Annotate |
Now that MIR is a thing, it would be nice if this were implemented. |
@comex I do agree =) @Aatch Not sure what your time is like, but if you don't have time to implement this, it seems like a case where we might be able to mentor someone. The idea would be to leave a comment explaining roughly how you think it should work, and then tagging the issue with |
Unassigning @Aatch since, empirically, he doesn't seem to be actively hacking on this right now. =) (Feel free to correct me.) |
@nikomatsakis PR up, though I could use some help adding tests. |
Copying @nagisa's comment from #36181:
I think that keeping the intrinsics is a good idea. They should be fairly obvious for people coming from C/C++. Attributes aren't mutually exclusive with the intrinsics, so we're not exactly cutting ourselves off with intrinsics. Supporting branch weights at the MIR level would probably be the easiest implementation for lowering to the metadata ourselves. |
core: add likely and unlikely intrinsics I'm no good at reading assembly, but I have tried a stage1 compiler with this patch, and it does cause different asm output. Additionally, testing this compiler on my httparse crate with some `likely` usage added in to the branches does affect benchmarks. However, I'm sure a codegen test should be included, if anyone knows what it should look like. There isn't an entry in `librustc_trans/context.rs` in this diff, because it already exists (`llvm.expect.i1` is used for array indices). ---- Even though this does affect httparse benchmarks, it doesn't seem to affect it the same way GCC's `__builtin_expect` affects picohttpparser. I was confused that the deviation on the benchmarks grew hugely when testing this, especially since I'm absolutely certain that the branchs where I added `likely` were always `true`. I chalk that up to GCC and LLVM handle branch prediction differently. cc #26179
Just adding a note here as I'm processing everything in #38643: it will need docs before stabilization. @nikomatsakis would you mind adding a note to that effect in the root of the issue? |
@chriskrycho done |
Likely unlikely fix RFC 1131 ( rust-lang/rust#26179 ) added likely/unlikely intrinsics, but they have been broken for a while: rust-lang/rust#96276 , rust-lang/rust#96275 , rust-lang/rust#88767 . This PR tries to fix them. Changes: - added a new `cold_path()` intrinsic - `likely()` and `unlikely()` changed to regular functions implemented using `cold_path()`
Proposal to reexport |
Reexport likely/unlikely in std::hint Since `likely`/`unlikely` should be working now, we could reexport them in `std::hint`. I'm not sure if this is already approved or if it requires approval Tracking issue: rust-lang#26179
Reexport likely/unlikely in std::hint Since `likely`/`unlikely` should be working now, we could reexport them in `std::hint`. I'm not sure if this is already approved or if it requires approval Tracking issue: rust-lang#26179
Rollup merge of rust-lang#133695 - x17jiri:hint_likely, r=Amanieu Reexport likely/unlikely in std::hint Since `likely`/`unlikely` should be working now, we could reexport them in `std::hint`. I'm not sure if this is already approved or if it requires approval Tracking issue: rust-lang#26179
These currently point to rust-lang#26179, which is nearly a decade old and has a lot of outdated discussion. Move these features to a new tracking issue specifically for the recently added API.
These currently point to rust-lang#26179, which is nearly a decade old and has a lot of outdated discussion. Move these features to a new tracking issue specifically for the recently added API. New tracking issue: rust-lang#136873
We now have |
… r=jhpratt Change the issue number for `likely_unlikely` and `cold_path` These currently point to rust-lang#26179, which is nearly a decade old and has a lot of outdated discussion. Move these features to a new tracking issue specifically for the recently added API. New tracking issue: rust-lang#136873
… r=jhpratt Change the issue number for `likely_unlikely` and `cold_path` These currently point to rust-lang#26179, which is nearly a decade old and has a lot of outdated discussion. Move these features to a new tracking issue specifically for the recently added API. New tracking issue: rust-lang#136873
Rollup merge of rust-lang#136874 - tgross35:likely-unlikely-tracking, r=jhpratt Change the issue number for `likely_unlikely` and `cold_path` These currently point to rust-lang#26179, which is nearly a decade old and has a lot of outdated discussion. Move these features to a new tracking issue specifically for the recently added API. New tracking issue: rust-lang#136873
These currently point to rust-lang#26179, which is nearly a decade old and has a lot of outdated discussion. Move these features to a new tracking issue specifically for the recently added API. New tracking issue: rust-lang#136873
These currently point to rust-lang#26179, which is nearly a decade old and has a lot of outdated discussion. Move these features to a new tracking issue specifically for the recently added API. New tracking issue: rust-lang#136873
These currently point to rust-lang#26179, which is nearly a decade old and has a lot of outdated discussion. Move these features to a new tracking issue specifically for the recently added API. New tracking issue: rust-lang#136873
These currently point to rust-lang#26179, which is nearly a decade old and has a lot of outdated discussion. Move these features to a new tracking issue specifically for the recently added API. New tracking issue: rust-lang#136873
These currently point to rust-lang#26179, which is nearly a decade old and has a lot of outdated discussion. Move these features to a new tracking issue specifically for the recently added API. New tracking issue: rust-lang#136873
These currently point to rust-lang#26179, which is nearly a decade old and has a lot of outdated discussion. Move these features to a new tracking issue specifically for the recently added API. New tracking issue: rust-lang#136873
These currently point to rust-lang#26179, which is nearly a decade old and has a lot of outdated discussion. Move these features to a new tracking issue specifically for the recently added API. New tracking issue: rust-lang#136873
These currently point to rust-lang#26179, which is nearly a decade old and has a lot of outdated discussion. Move these features to a new tracking issue specifically for the recently added API. New tracking issue: rust-lang#136873
Reexport likely/unlikely in std::hint Since `likely`/`unlikely` should be working now, we could reexport them in `std::hint`. I'm not sure if this is already approved or if it requires approval Tracking issue: rust-lang#26179
These currently point to rust-lang#26179, which is nearly a decade old and has a lot of outdated discussion. Move these features to a new tracking issue specifically for the recently added API. New tracking issue: rust-lang#136873
… r=jhpratt Change the issue number for `likely_unlikely` and `cold_path` These currently point to rust-lang#26179, which is nearly a decade old and has a lot of outdated discussion. Move these features to a new tracking issue specifically for the recently added API. New tracking issue: rust-lang#136873
These currently point to rust-lang#26179, which is nearly a decade old and has a lot of outdated discussion. Move these features to a new tracking issue specifically for the recently added API. New tracking issue: rust-lang#136873
… r=jhpratt Change the issue number for `likely_unlikely` and `cold_path` These currently point to rust-lang#26179, which is nearly a decade old and has a lot of outdated discussion. Move these features to a new tracking issue specifically for the recently added API. New tracking issue: rust-lang#136873
These currently point to rust-lang#26179, which is nearly a decade old and has a lot of outdated discussion. Move these features to a new tracking issue specifically for the recently added API. New tracking issue: rust-lang#136873
Tracking and meta discussion for branch and path hinting, including:
likely
/unlikely
as accepted in Add an expect intrinsic rfcs#1131 (much has changed since this RFC)core::hint::{likely, unlikely}
Tracking Issue forlikely_unlikely
andcold_path
#136873core::hint::cold_path
Tracking Issue forlikely_unlikely
andcold_path
#136873bool::select_unpredictable
Tracking Issue forbool::select_unpredictable
#133962#[cold]
:The text was updated successfully, but these errors were encountered: