Skip to content

fix exp in --math-mode=fast#41600

Closed
oscardssmith wants to merge 2 commits intoJuliaLang:masterfrom
oscardssmith:oscardssmith-safer-fastmath-exp
Closed

fix exp in --math-mode=fast#41600
oscardssmith wants to merge 2 commits intoJuliaLang:masterfrom
oscardssmith:oscardssmith-safer-fastmath-exp

Conversation

@oscardssmith
Copy link
Copy Markdown
Member

@oscardssmith oscardssmith commented Jul 15, 2021 •
edited by stevengj
Loading

don't rely on muladd fusing. Performance impact is minimal (max 1 cycle regression)

Closes #41592

@oscardssmith
fix exp in --math-mode=fast
5124189 
don't rely on `muladd` fusing.
@oscardssmith oscardssmith added maths Mathematical functions embarrassing-bugfix Whoops! labels Jul 15, 2021
@vtjnash vtjnash added design Design of APIs or of the language itself and removed embarrassing-bugfix Whoops! labels Jul 15, 2021
@oscardssmith
Copy link
Copy Markdown
Member Author

oscardssmith commented Jul 15, 2021

Merging soon if there are no objections.

@DilumAluthge
Copy link
Copy Markdown
Member

DilumAluthge commented Jul 15, 2021

Would be good to get a review?

@DilumAluthge DilumAluthge requested review from stevengj and vtjnash July 15, 2021 23:25
@oscardssmith
Copy link
Copy Markdown
Member Author

oscardssmith commented Jul 15, 2021

probably

@Keno
Copy link
Copy Markdown
Member

Keno commented Jul 16, 2021

I don't think we really should be designing for --math-mode=fast. That flag is just broken.

@oscardssmith
Copy link
Copy Markdown
Member Author

oscardssmith commented Jul 16, 2021

This is actually fixing a broader issue that I had incorrectly assumed muladd would fold to fma here, and when it doesn't, it produces broken results. Since there is a 0 cost work around (or very close, within noise), this is still worth it.

@vtjnash
Copy link
Copy Markdown
Member

vtjnash commented Jul 16, 2021

It appears to still fail tests on all platforms, so there is some work left

When you say "assumed to be fma" do you mean we should have actually called the fma function here? Or just that the performance advantage of fma is tiny, but the loss of performance and accuracy (if it doesn't fold to fma) is undesirable?

@oscardssmith
Copy link
Copy Markdown
Member Author

oscardssmith commented Jul 16, 2021

so I was using an fma with a subtraction to round, but I can also just round instead. The failing test appears to be NaN which was working implicitly before, but failing now.

@PallHaraldsson PallHaraldsson mentioned this pull request Jul 16, 2021
Closed
@kimikage
Copy link
Copy Markdown
Contributor

kimikage commented Jul 16, 2021

To be clear, this problem is not directly caused by fma. This is an optimization problem.

@KristofferC
Copy link
Copy Markdown
Member

KristofferC commented Jul 16, 2021

What about

julia/base/special/exp.jl

Lines 404 to 406 in 2893de7

N_float = muladd(x, LogBo256INV(Val(:ℯ), T), MAGIC_ROUND_CONST(T))
N = reinterpret(UInt64, N_float) % Int32
N_float -= MAGIC_ROUND_CONST(T) #N_float now equals round(x*LogBo256INV(Val(:ℯ), T))

@oscardssmith
Copy link
Copy Markdown
Member Author

oscardssmith commented Jul 16, 2021

I just realized that the problem here wasn't muladd not folding, but fastmath making an assumption that x+a-a=x. Given that, this can just be closed as math mode fast being broken.

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

Reviewers

@stevengj stevengj Awaiting requested review from stevengj

@vtjnash vtjnash Awaiting requested review from vtjnash

Assignees

No one assigned

Labels

design Design of APIs or of the language itself maths Mathematical functions

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

exp() accuracy is severly affected when using --fast-mode=fast

6 participants

@oscardssmith @DilumAluthge @Keno @vtjnash @kimikage @KristofferC