Make all integer intrinsics generic #29316
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| extern "rust-intrinsic" { | ||
| /// Performs an unchecked division, resulting in undefined behavior | ||
| /// where y = 0 or x = `T::min_value()` and y = -1 | ||
| pub fn unchecked_div<T>(x: T, y: T) -> T; |
There was a problem hiding this comment.
I think these can be in the same place they were in above, the #[cfg] should work ok on the functions as well as the block itself.
|
@alexcrichton alright, I made the changes you suggested. |
|
Thanks! Could you also squash to one commit now? Otherwise r=me |
|
@alexcrichton this has blown up far more. I've replaced all integer intrinsics with generics, and do this same thing with all of them (And added two errors in the process, one for calling bswap with u/i8, and the other is the error you see in this PR). |
| ast::TyU64 => 64, | ||
| }; | ||
| match c { | ||
| "ctlz" => count_zeros_intrinsic(bcx, &format!("llvm.ctlz.i{}", width), |
There was a problem hiding this comment.
It seems like the int/uint branches here share quite a bit of code, perhaps they could be deduplicated?
There was a problem hiding this comment.
Unfortunately no, as they are two different types. The most I could do is unduplicate the target_pointer_width part.
There was a problem hiding this comment.
Hm are you sure? It looks like everything is basically duplicated except for unsigned/signed in a few locations which could be a parameter to a shared function? For example the bswap intrinsic has exactly the same code in both branches here.
strega-nil
changed the title
| int_impl! { i8, u8, 8, | ||
| intrinsics::add_with_overflow, | ||
| intrinsics::sub_with_overflow, | ||
| intrinsics::mul_with_overflow } |
There was a problem hiding this comment.
Since these are all the same, can't the macro just not take them anymore when #[cfg(not(stage0))]?
There was a problem hiding this comment.
Yes, but the macro is 500 lines long. It's better to add it here until we get these into stage0, and then change the macro.
There was a problem hiding this comment.
Did not realize that, makes sense.
|
LGTM modulo nits. |
|
Alright, I think I've covered all your nits @eddyb? |
|
@bors r+ |
|
📌 Commit 29808c4 has been approved by |
|
⌛ Testing commit 29808c4 with merge 10a4130... |
|
💔 Test failed - auto-linux-64-opt |
|
Blargh, fixing. |
|
hopefully fixed :'( |
|
@bors r+ |
|
📌 Commit 93ca0fa has been approved by |
Similarly to the simd intrinsics. I believe this is a better solution than #29288, and I could implement it as well for overflowing_add/sub/mul. Also rename from udiv/sdiv to div, and same for rem.
|
💔 Test failed - auto-mac-32-opt |
Similarly to the simd intrinsics.
|
alright should be fixed for reals this time :'( |
|
@bors r+ |
|
📌 Commit 579420f has been approved by |
Similarly to the simd intrinsics. I believe this is a better solution than #29288, and I could implement it as well for overflowing_add/sub/mul. Also rename from udiv/sdiv to div, and same for rem.
Similarly to the simd intrinsics. I believe this is a better solution than #29288, and I could implement it as well for overflowing_add/sub/mul. Also rename from udiv/sdiv to div, and same for rem.