Skip to content

Unchecked div/rem no longer generic #29288

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

Closed
wants to merge 1 commit into from
Closed

Unchecked div/rem no longer generic #29288

wants to merge 1 commit into from

Conversation

strega-nil
Copy link
Contributor

Now they are [ui](8|16|32|64)_unchecked_(div|rem)

This is more in line with the other mathematical intrinsics, and means you can't, for example, call unchecked_sdiv((),()) anymore.

Now they are [ui](8|16|32|64)_unchecked_(div|rem)
@rust-highfive
Copy link
Contributor

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@strega-nil
Copy link
Contributor Author

I'd also like to change overflowing_(add|sub|mul) to be non-generic if possible.

@nagisa
Copy link
Member

nagisa commented Oct 25, 2015

Rather than this approach, I’d prefer intrinsics checked their specialisation instead. Similarly to the SSE intrinsics code. See this which results in E511.

@alexcrichton
Copy link
Member

I would also kinda tend to err on the side of ease of implementation of intrinsics, but @GBGamer I'm curious was there a use case other than bad monomorphizations which motivated this?

@strega-nil
Copy link
Contributor Author

@alexcrichton because it's really ugly. unchecked_udiv<T>(T, T) -> T says "Hey, I'm valid for all T, so pass anything to me." when really, it's exclusively for the integer types. It's not meant for anything else.

@huonw
Copy link
Member

huonw commented Oct 25, 2015

Intrinsics are regarded as things that should essentially never be called directly: just once for each type, to build up safe abstractions. I.e. inaccuracies in their type signatures aren't thought to be the end of the world.

@strega-nil
Copy link
Contributor Author

@huonw: that may be true, that doesn't mean it doesn't bother me :P.

However, I think that switching to the simd_add<T> version would be good. Then we don't have to worry about an LLVM error popping up in rust code, and there's no bloat. Actually, this might be good for all of the other intrinsics that are currently T_function, like u8_mul_with_overflow and friends.

@strega-nil
Copy link
Contributor Author

@huonw The issue I have is this:

#![feature(core_intrinsics)]
fn main() {
    unsafe { std::intrinsics::unchecked_sdiv((),()); }
}

ends up spitting out

/Constants.cpp:1898: static llvm::Constant* llvm::ConstantExpr::get(unsigned int, llvm::Constant*, llvm::Constant*, unsigned int, llvm::Type*): Assertion `C1->getType()->isIntOrIntVectorTy() && "Tried to create an arithmetic operation on a non-arithmetic type!"' failed.
Aborted (core dumped)

and also that you can call sdiv on an unsigned integer, and udiv on a signed integer.

@huonw
Copy link
Member

huonw commented Oct 26, 2015

Sure, I'm all for "last minute" (i.e. like simd_add) checks to avoid LLVM asserts like that.

@strega-nil
Copy link
Contributor Author

I've an idea. One sec, let me try something.

@strega-nil strega-nil closed this Oct 26, 2015
bors added a commit that referenced this pull request Oct 31, 2015
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.
bors added a commit that referenced this pull request Nov 1, 2015
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.
@strega-nil strega-nil deleted the changed-unchecked-div branch September 11, 2018 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants