Improved assumptions relating to isqrt#155265
Improved assumptions relating to isqrt#155265Apersoma wants to merge 1 commit intorust-lang:mainfrom
Conversation
rustbot
added
S-waiting-on-review
|
r? @jhpratt rustbot has assigned @jhpratt. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? tgross35 |
|
Could you include a godbolt link or before+after asm outputs indicating what all improves here? |
| // Since these are integers, the first implies the second | ||
| // (though the compiler is too stupid to realize this). | ||
| unsafe { | ||
| crate::hint::assert_unchecked(result * result <= $n); |
There was a problem hiding this comment.
It might be a good idea to use unchecked_mul instead of *
There was a problem hiding this comment.
It might be a good idea to use
unchecked_mulinstead of*
It doesn't change anything in my testing, but I'll do that since small changes to the compiler might end up affecting that. It also shouldn't have any effects if the compiler is smart enough (because of the result <= isqrt(MAX)), but so should the one right below it and that one does anyway.
There was a problem hiding this comment.
Accidentally reviewed at the wrong PR, copied the comments over from #154115 (comment).
| /// Takes the isqrt of `$n` assuming its non-negative. | ||
| /// | ||
| /// # Safety: | ||
| /// | ||
| /// `$n` must be nonnegative | ||
| macro_rules! isqrt_of_nonnegative { |
There was a problem hiding this comment.
Can't this all live in the uint implementations, then signed integers cast to unsigned (after the negative check) and call that? To avoid the extra macro here.
MAX_RESULT is different but that can just be an additional (narrower) assertion for signed integers. And you can do the >=0 assertion there since it's not meaningful for unsigned.
| if $n > 0 { | ||
| crate::hint::assert_unchecked(result > 0); | ||
| } |
There was a problem hiding this comment.
This is part of the macro's precondition, no?
There was a problem hiding this comment.
the macro's precondition is $n >= 0.
| crate::hint::assert_unchecked(result * result <= $n); | ||
| crate::hint::assert_unchecked(result <= $n); |
There was a problem hiding this comment.
Aren't these two redundant? If not, could you show an asm case indicating that?
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
5 participants
Improved various assumptions relating to values yielded by
isqrt.Does not solve but does improve #132763.
Re-openeing of #154115
Added assumptions are:
xis nonzero thenx.isqrt()is nonzerox.isqrt() <= xx.isqrt() * x.isqrt() <= x