-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fix digits(n::Unsigned) with neg base for n > typemax(n)÷2
#29205
Conversation
731f384
to
0a9bc41
Compare
# manually peel one loop iteration for type stability | ||
n, d = fldmod(n, -base) | ||
a[firstindex(a)] = d | ||
n = -signed(n) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is type unstable, but maybe the compiler is good enough to handle that, or that the the cost of the divisions makes this negligible?
EDIT: after some very basic benchmarking, this doesn't seem indeed to have any significant performance impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This used to be type unstable but since the introduction of SSAIR the compiler doesn't care about the reuse of variable names at all (that's basically what the SS part means: single static assignment so reassigning amounts to just assigning a different variable). It only matters if the same "slot" in a basic block can have different types, e.g. in the loop body below. Even then the union splitting can deal with it fairly often.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this explanation, that's very good to know, it allows not bothering with this kind of gymnastic!
0a9bc41
to
4fb103a
Compare
Alternative based on #29187 Tests from rforquet's PR linked above.
4fb103a
to
15ff960
Compare
Alternative based on #29187