-
-
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
Add mul_hi function for bit integers #57276
base: master
Are you sure you want to change the base?
Conversation
Move the _mul_high function from base/multinverses.jl to base/int.jl. Rename it to mul_hi.
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.
Looks great! while we're at it, do we want to use this to make widemul
of UInt128
/Int128
faster (by doing a mul, a mulhi and building the BigInt manually)? I guess that can always go in a followup PR if we care.
I am not sure: I guess dynamic allocation would dominate the cost. |
Should |
I wouldn't for now. Exporting is easy if we want to in the future. |
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.
Lovely, thanks!
Good choice re-using the existing implementation.
This should also be marked public in base/public.jl
, even if not exported.
Co-authored-by: Lilith Orion Hafner <[email protected]>
I'd suspect that on a 32 bit platform, the performance of |
LLVM should be able to figure something decent out |
Co-authored-by: Lilith Orion Hafner <[email protected]>
I don't know if this counts as bad enough, although the native code is a bit overcomplicated:
.text
.file "mul_hi"
.globl julia_mul_hi_5686 # -- Begin function julia_mul_hi_5686
.p2align 4, 0x90
.type julia_mul_hi_5686,@function
julia_mul_hi_5686: # @julia_mul_hi_5686
; Function Signature: mul_hi(UInt64, UInt64)
# %bb.0: # %top
push ebp
mov ebp, esp
push ebx
push edi
push esi
#DEBUG_VALUE: mul_hi:a <- [DW_OP_plus_uconst 12] [$ebp+0]
#DEBUG_VALUE: mul_hi:b <- [DW_OP_plus_uconst 20] [$ebp+0]
mov ecx, dword ptr [ebp + 20]
mov eax, dword ptr [ebp + 24]
mov esi, dword ptr [ebp + 12]
mov edx, ecx
mulx ebx, ebx, esi
mov edx, eax
mulx esi, edi, esi
add edi, ebx
mov ebx, dword ptr [ebp + 16]
adc esi, 0
mov edx, ecx
mulx ecx, eax, ebx
mov edx, dword ptr [ebp + 24]
mulx ebx, edx, ebx
add eax, edi
adc ecx, esi
setb al
add edx, ecx
movzx eax, al
adc ebx, eax
mov eax, dword ptr [ebp + 8]
mov dword ptr [eax], edx
mov dword ptr [eax + 4], ebx
pop esi
pop edi
pop ebx
pop ebp
ret 4
.Lfunc_end0:
.size julia_mul_hi_5686, .Lfunc_end0-julia_mul_hi_5686
# -- End function
.section ".note.GNU-stack","",@progbits |
@@ -71,6 +71,9 @@ public | |||
isoperator, | |||
isunaryoperator, | |||
|
|||
# Integer math | |||
mul_hi, |
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.
If this is going to be public, it'd be nice for it to have a more descriptive name (and ideally one that is stylistically consistent, i.e. germanic case rather than snake case). Perhaps mulhighhalf
or something like that?
Move the
_mul_high
function from base/multinverses.jl to base/int.jl. Rename it tomul_hi
.Addresses #53855.
I chose to reuse existing implementation over using the one proposed in #53855 by @LilithHafner because their performance is similar on my PC.