Skip to content

Conversation

@11happy
Copy link
Contributor

@11happy 11happy commented Jan 18, 2024

Overview:

Testing:

  • Tested the updated code.
  • Verified that other functionalities remain unaffected.

Dependencies:

  • No dependencies on other pull requests.

CC:

@barucden
Copy link
Contributor

Are the other methods (for Int32 and Int64) any better than this fallback? I see that the one for Int32 omits the condition, but the one for Int64 seems exactly like the newly-added, general method.

@11happy
Copy link
Contributor Author

11happy commented Jan 18, 2024

Are the other methods (for Int32 and Int64) any better than this fallback? I see that the one for Int32 omits the condition, but the one for Int64 seems exactly like the newly-added, general method.

like there's not much significant difference, Int32 just omits the condition.

@barucden
Copy link
Contributor

The method for Int32 avoids the check x == Float64(x) likely because Int32 is always representable as Float64 exactly. The same holds even for Int8 and Int16, right? We might extend that method to accept Union{Int8, Int16, Int32}.

@ViralBShah
Copy link
Member

cc @oscardssmith

@ViralBShah ViralBShah added the maths Mathematical functions label Jan 18, 2024
@11happy
Copy link
Contributor Author

11happy commented Jan 18, 2024

The method for Int32 avoids the check x == Float64(x) likely because Int32 is always representable as Float64 exactly. The same holds even for Int8 and Int16, right? We might extend that method to accept Union{Int8, Int16, Int32}.

sure could do that , should I?

Signed-off-by: 11happy <[email protected]>
@N5N3
Copy link
Member

N5N3 commented Jan 19, 2024

sure could do that , should I?

I guess this is not a must. LLVM is clever enough to eliminate the error path. (So perhaps there's no need to keep the Int32 dispatch)

julia> function myrem2pi(x::Integer, r::RoundingMode)                                          
           fx = float(x)                                                                       
               fx == x || throw(ArgumentError(LazyString(typeof(x), " argument to rem2pi is too large: ", x)))                                                                                
           rem2pi(fx, r)                                                                       
       end
myrem2pi (generic function with 1 method)

julia> @code_llvm myrem2pi(Int32(1), RoundUp)
; Function Signature: myrem2pi(Int32, Base.Rounding.RoundingMode{:Up})
;  @ REPL[1]:1 within `myrem2pi`
; Function Attrs: uwtable
define double @julia_myrem2pi_6707(i32 signext %"x::Int32") #0 {
L5:
;  @ REPL[1]:2 within `myrem2pi`
; ┌ @ float.jl:356 within `float`
; │┌ @ float.jl:329 within `AbstractFloat`
; ││┌ @ float.jl:221 within `Float64`
     %0 = sitofp i32 %"x::Int32" to double
; └└└
;  @ REPL[1]:4 within `myrem2pi`
  %1 = call double @j_rem2pi_6713(double %0)
  ret double %1
}

@N5N3 N5N3 added the merge me PR is reviewed. Merge when all tests are passing label Jan 20, 2024
@barucden
Copy link
Contributor

What about removing the Int32 method then, as you suggested @N5N3?

@N5N3 N5N3 merged commit a9e2bd4 into JuliaLang:master Jan 21, 2024
@N5N3 N5N3 removed the merge me PR is reviewed. Merge when all tests are passing label Jan 21, 2024
@N5N3
Copy link
Member

N5N3 commented Jan 21, 2024

@barucden That make sense to me. Perhaps we need some test to ensure there's no regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maths Mathematical functions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rem2pi(x::Integer, r::RoundingMode) is missing.

5 participants