Skip to content

implement fp32 math functions #2044

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Kaida-Amethyst
Copy link
Contributor

@Kaida-Amethyst Kaida-Amethyst commented Apr 30, 2025

For closing #1069

Copy link

peter-jerry-ye-code-review bot commented Apr 30, 2025

Missing description for important implementation-specific constants

Category
Maintainability
Code Snippet
let b1 : UInt = 709958130 // B1 = (127-127.0/3-0.03306235651)*2**23 */
Recommendation
Add detailed documentation comments explaining the mathematical constants, their purpose and derivation
Reasoning
Magic numbers and mathematical constants need clear documentation for future maintainability. These constants are critical for the floating point implementation but their meaning is not immediately clear.

Potential precision loss in chained floating point operations

Category
Performance
Code Snippet
w = z * z
r = z * ivln2_l - w * ivln2
Recommendation
Consider using fused multiply-add (FMA) operations where available to reduce rounding errors
Reasoning
Chained floating point operations can accumulate rounding errors. Using FMA when possible would improve numerical stability while maintaining performance.

Edge case handling in trig_reduce function could be improved

Category
Correctness
Code Snippet
phi = (phi << 1) | (plo >> 31)
plo = plo << 1
exp = exp - 1
Recommendation
Add explicit bounds checks to prevent potential integer overflow in bit shift operations
Reasoning
The bit manipulation code assumes certain ranges for values. Adding explicit checks would make the code more robust against edge cases and invalid inputs.

Copy link

Potential integer overflow in mulh function

Category
Correctness
Code Snippet
fn mulh(a : UInt, b : UInt) -> UInt {
let a = a.to_uint64()
let b = b.to_uint64()
let res = a * b
(res >> 32).to_uint()
}
Recommendation
Add range checks to ensure input values don't cause overflow in uint64 multiplication
Reasoning
Multiplying two 32-bit integers can produce a result larger than 64 bits. While this may be intended for this specific trigonometric usage, adding guards would prevent misuse in other contexts.

Magic numbers in trigonometric functions lack explanatory comments

Category
Maintainability
Code Snippet
const SIN_SWITCHOVER : Float = 201.15625
const COS_SWITCHOVER : Float = 142.90625
Recommendation
Add detailed comments explaining the mathematical significance of these constants and how they were derived
Reasoning
These magic numbers appear to be important thresholds for algorithm selection, but their derivation and purpose is not documented, making future maintenance difficult.

Redundant conditional check in pow function

Category
Performance
Code Snippet
if ix == 0x7f800000 || ix == 0 || ix == 0x3f800000 {
// x is +-0,+-inf,+-1
z = ax
if hy < 0 {
// z = (1/|x|)
z = (1.0 : Float) / z
}
Recommendation
Consider combining special case checks to reduce branching
Reasoning
Multiple separate equality checks against common special values could be consolidated to improve branch prediction and reduce instruction count.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 6540

Details

  • 147 of 225 (65.33%) changed or added relevant lines in 8 files are covered.
  • 14 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-1.1%) to 91.376%

Changes Missing Coverage Covered Lines Changed/Added Lines %
float/hypot.mbt 3 4 75.0%
float/scalbn.mbt 6 8 75.0%
float/hyperbolic.mbt 37 40 92.5%
float/log.mbt 8 12 66.67%
float/exp.mbt 17 22 77.27%
float/pow.mbt 21 47 44.68%
float/trig.mbt 50 87 57.47%
Files with Coverage Reduction New Missed Lines %
double/exp_nonjs.mbt 2 82.69%
float/log.mbt 4 55.56%
float/exp.mbt 8 59.38%
Totals Coverage Status
Change from base Build 6539: -1.1%
Covered Lines: 6230
Relevant Lines: 6818

💛 - Coveralls

@coveralls
Copy link
Collaborator

coveralls commented Apr 30, 2025

Pull Request Test Coverage Report for Build 6548

Details

  • 147 of 248 (59.27%) changed or added relevant lines in 14 files are covered.
  • 14 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-1.4%) to 91.069%

Changes Missing Coverage Covered Lines Changed/Added Lines %
float/hypot.mbt 3 4 75.0%
float/scalbn.mbt 6 8 75.0%
math/algebraic.mbt 0 2 0.0%
math/exp.mbt 0 2 0.0%
math/log.mbt 0 2 0.0%
float/hyperbolic.mbt 37 40 92.5%
float/log.mbt 8 12 66.67%
math/round.mbt 0 4 0.0%
float/exp.mbt 17 22 77.27%
math/hyperbolic.mbt 0 6 0.0%
Files with Coverage Reduction New Missed Lines %
double/exp_nonjs.mbt 2 82.69%
float/log.mbt 4 55.56%
float/exp.mbt 8 59.38%
Totals Coverage Status
Change from base Build 6539: -1.4%
Covered Lines: 6230
Relevant Lines: 6841

💛 - Coveralls

@Kaida-Amethyst Kaida-Amethyst force-pushed the ziyue/fp32_math branch 2 times, most recently from 3da836a to 547ab1c Compare April 30, 2025 07:57
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.

2 participants