Skip to content
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

disagreement in converting float32 result to float64 on 32-bit platform #41

Closed
JeffBezanson opened this issue Jun 11, 2011 · 15 comments
Closed
Labels
bug Indicates an unexpected problem or unintended behavior system:32-bit Affects only 32-bit systems

Comments

@JeffBezanson
Copy link
Sponsor Member

This was seen on a 32-bit Xeon system:

julia> ndigf(n)=float64(log(float32(n)))

julia> ndigf(256)
5.5451774444736657

julia> float64(log(float32(256)))
5.5451774597167969

julia> log(256)
5.5451774444795623

The last answer is correct, the middle one is the float64 conversion of the correctly-rounded float32 answer, and the first one is something strange.

julia> num2hex(5.5451774444795623)
"40162e42fefa39ef"

julia> num2hex(5.5451774444736657)
"40162e42fefa2000"

Looks like the float64 answer with the last 13 bits zero'd.

Happens with some functions, e.g. log and sin, but not with sqrt.

@ViralBShah
Copy link
Member

Might this have anything to do with the gcc optimization bug mentioned in the fdlibm readme?

@JeffBezanson
Copy link
Sponsor Member Author

Very likely. I tried the libm version and it doesn't have this issue.

@StefanKarpinski
Copy link
Sponsor Member

Jeff, can you check if this is fixed by compiling fdlibm with -O1 instead of -O2?

@JeffBezanson
Copy link
Sponsor Member Author

-O1 did not fix this, but it did fix the other one.

@JeffBezanson
Copy link
Sponsor Member Author

What am I smoking, it can't be the same bug, since this is calling logf, the float32 version, which already has the pointer cast bug fixed.

@JeffBezanson
Copy link
Sponsor Member Author

OK, this is just the classic extended precision issue. I can fix it by replacing

return y;

with

volatile float x;
x=y; return x;

in the fdlibm code.
Inside ndigf's llvm-generated code the result stays in an extended-precision register and so can be converted to float64 without first rounding to float32.
So what should we do? I could modify our code generator to force rounding of results returned by C functions.

@StefanKarpinski
Copy link
Sponsor Member

I think always rounding is the right thing to do. Consistency and determinism is more important than keeping extra bits around. This same issue has already caused both Java and PHP to have DOS bugs in the past few months, let's learn the lesson from them.

@ViralBShah
Copy link
Member

I agree that let's go for consistency. That crlibm presentation discusses this issue.

Best to also check with Alan on this issue. He is probably not reading this thread.

-viral

On Jun 15, 2011, at 12:41 PM, StefanKarpinski wrote:

I think always rounding is the right thing to do. Consistency and determinism is more important than keeping extra bits around. This same issue has already caused both Java and PHP to have DOS bugs in the past few months, let's learn the lesson from them.

Reply to this email directly or view it on GitHub:
#41 (comment)

@StefanKarpinski
Copy link
Sponsor Member

He can't actually read this thread — this is private and AFAIK, Alan doesn't have a GitHub account.

@ArchRobison
Copy link
Contributor

Thanks for adding the test. Alas on i386 systems, using bitcast instead of the volatile store/load sequence flunks the test. I'll poke around some more.

@Keno
Copy link
Member

Keno commented Jul 23, 2014

We should bring this up on the llvm list.

@ArchRobison
Copy link
Contributor

I concur, and sent a note to llvmdev@cs.uiuc.edu.

@JeffBezanson
Copy link
Sponsor Member Author

Maybe try bitcasting through an int32?

@ArchRobison
Copy link
Contributor

I think we're going to end up wanting to take different actions depending upon whether the target has excess precision, with the goal of presenting least obfuscated code to the LLVM optimizer. I'm guessing that we can detect the presence of excess precision from LLVM somehow since Clang must detect it to set FLT_EVAL_METHOD for C99.

[pao: fixed link to FLT_EVAL_METHOD]

@JeffBezanson
Copy link
Sponsor Member Author

It seems this is not an issue at all on 64-bit, so maybe the first move is just to disable this hack on anything x86-64.

ArchRobison pushed a commit to ArchRobison/julia that referenced this issue Jul 24, 2014
ArchRobison pushed a commit to ArchRobison/julia that referenced this issue Jul 24, 2014
…g#41

to use volatile store/load on 32-bit x86 only.
ArchRobison pushed a commit to ArchRobison/julia that referenced this issue Jul 24, 2014
…g#41

to use volatile store/load on 32-bit x86 only.
ArchRobison pushed a commit to ArchRobison/julia that referenced this issue Jul 24, 2014
…g#41

to use volatile store/load on 32-bit x86 only.
JeffBezanson added a commit that referenced this issue Jul 24, 2014
Fix issue #7687, by revising original fix for issue #41
StefanKarpinski pushed a commit that referenced this issue Feb 8, 2018
StefanKarpinski pushed a commit that referenced this issue Feb 8, 2018
cmcaine pushed a commit to cmcaine/julia that referenced this issue Sep 24, 2020
Fix atbash-cipher test sets indentation
Keno pushed a commit that referenced this issue Oct 9, 2023
Fix world age problems when running tests
topolarity added a commit that referenced this issue May 6, 2024
Without this change, the compiler fails to notice that
`env_threads isa Int` in the fall-through case, leading to a union-split
with a branch that is in fact unreachable:

```
43 ┄ %109 = φ (#41 => %105, #42 => %108)::Union{Nothing, Int64}
│    %110 = (%109 isa Int64)::Bool
└───        goto #45 if not %110
...
45 ─ %126 = π (%109, Nothing)
│           Base.convert(Int64, %126)::Union{}
└───        unreachable
```

Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
aviatesk pushed a commit that referenced this issue May 7, 2024
Without this change, the compiler fails to notice that `env_threads isa
Int` in the fall-through case, leading to a union-split with a branch
that is in fact unreachable:

```
43 ┄ %109 = φ (#41 => %105, #42 => %108)::Union{Nothing, Int64}
│    %110 = (%109 isa Int64)::Bool
└───        goto #45 if not %110
...
45 ─ %126 = π (%109, Nothing)
│           Base.convert(Int64, %126)::Union{}
└───        unreachable
```

After this change, the union-split is eliminated.

Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
lazarusA pushed a commit to lazarusA/julia that referenced this issue Jul 12, 2024
Without this change, the compiler fails to notice that `env_threads isa
Int` in the fall-through case, leading to a union-split with a branch
that is in fact unreachable:

```
43 ┄ %109 = φ (JuliaLang#41 => %105, JuliaLang#42 => %108)::Union{Nothing, Int64}
│    %110 = (%109 isa Int64)::Bool
└───        goto JuliaLang#45 if not %110
...
45 ─ %126 = π (%109, Nothing)
│           Base.convert(Int64, %126)::Union{}
└───        unreachable
```

After this change, the union-split is eliminated.

Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior system:32-bit Affects only 32-bit systems
Projects
None yet
Development

No branches or pull requests

5 participants