Skip to content
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

WIP implement guards in rem and div unsound cases #55

Merged
merged 1 commit into from
Feb 3, 2021

Conversation

miguelraz
Copy link
Contributor

@miguelraz miguelraz commented Jan 26, 2021

I'm leaving this WIP to address the rest of #54
I have to leave for a bit so I'm just opening this for now and will be back briefly to shape it up.

I'm doing this to get started with the contributing workflow and to test out my initial idea of just adding a panic! if any of the lanes match the int::MIN.

There's a lot of moving parts and I probably missed a lot of easy stuff, so please, all feedback is welcome.

@miguelraz
Copy link
Contributor Author

Thanks @thomcc !
This is my first contribution to the project so I'm still picking things up.
Concretely, is your comment proposing an API for future contributions, or to ammend this PR?

@thomcc
Copy link
Member

thomcc commented Jan 27, 2021

I'm mostly noting it for future reference.

@miguelraz
Copy link
Contributor Author

miguelraz commented Jan 27, 2021

Awesome, thanks for the input.
Thanks as well to @workingjubilee for inspecting the Travis failures on the webassembly builds #51 .

@thomcc
Copy link
Member

thomcc commented Jan 27, 2021

Given that it's not UB, that SIMD is a performance-oriented API, and that we don't panic for int overflow on adds and such (I think? or we'd have solved this already), it seems worth kicking the overflow checking in debug mode down the road a bit.

@scottmcm
Copy link
Member

and that we don't panic for int overflow on adds and such

This was my other question here. If neg is implemented in terms of sub, then it arguably should have already been doing the right thing (if perhaps with a suboptimal message).

But I also thing that it would be fine for you as a WG to decide that checked-in-debug isn't worth doing for simd, and define all the +/-/etc to always be wrapping. (I don't immediately know whether that's the right answer, but I assume you'll make a persuasive case for whichever direction you pick.)

@thomcc
Copy link
Member

thomcc commented Jan 27, 2021

But I also thing that it would be fine for you as a WG to decide that checked-in-debug isn't worth doing for simd

Yeah, I'd argue in favor of this, mostly because checked_add and stuff would be viable performantly (returning [Option<i32>; 4]...). I also think in practice people are more likely to want wrapping behavior on SIMD than normal integers. I think it should be a deliberate decision though.

@workingjubilee
Copy link
Member

While we're likely going to settle the issue regarding Neg some other day, that leaves the rest of issue #54, if you wish to continue to take this on. To recap, in the Zulip, I said:

"apply_binary_scalar_rhs_lanewise" is doing scalar ops, which of course panic.

so when that is removed and only the SIMD op is compiled in, running that test instead gets something like:

process didn't exit successfully: ~/projects/stdsimd/target/debug/deps/ops-6770e1bc9a8f6e58 (signal: 8, SIGFPE: erroneous arithmetic operation)

So the tests should be updated to not include the scalar comparisons, and instead just run the ops without checking the results, since we're only looking for a "successful" panic at runtime, not that the arithmetic results of the SIMD operation would match the arithmetic results of a comparable scalar operation.

@miguelraz
Copy link
Contributor Author

Tests now all pass, but the branch is still requesting a review from @workingjubilee from a resolved convo.
Re-requesting review.

@miguelraz miguelraz changed the title WIP neg(MIN) for ints fix WIP implement guards in rem and div unsound cases Feb 2, 2021
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better! My quibbles appear to be resolved and things appear to do what they say they do.

@miguelraz
Copy link
Contributor Author

Shoutout to Mario Carneiro for spotting a missing conditional.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhere in the review flow these were altered again.

@workingjubilee workingjubilee merged commit 9801540 into rust-lang:master Feb 3, 2021
@workingjubilee workingjubilee mentioned this pull request Feb 3, 2021
@miguelraz miguelraz deleted the min-panics branch February 3, 2021 01:21
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.

5 participants