add fenv cache to task struct #51288

simonbyrne · 2023-09-13T05:21:30Z

Fixes #51277. I know @vchuravy is skeptical, but I wanted to try it out.

To give some examples:

julia> t = Base.Rounding.setrounding_raw(Float64, Base.Rounding.to_fenv(RoundDown)) do
           Task(() -> println(rounding(Float64)))
       end
Task (runnable) @0x000000010dff04c0

julia> rounding(Float64)
RoundingMode{:Nearest}()

julia> wait(schedule(t))
RoundingMode{:Down}() # currently gives RoundingMode{:Nearest}()

julia> rounding(Float64)
RoundingMode{:Nearest}()

julia> Base.Rounding.setrounding_raw(Float64, Base.Rounding.to_fenv(RoundDown)) do
           Threads.@threads :static for i = 1:Threads.nthreads()
               println(Threads.threadid() =>  rounding(Float64))
           end
       end
1 => RoundingMode{:Down}()
2 => RoundingMode{:Down}() # currently gives RoundingMode{:Nearest}()
4 => RoundingMode{:Down}() # currently gives RoundingMode{:Nearest}()
3 => RoundingMode{:Down}() # currently gives RoundingMode{:Nearest}()

vtjnash

I think this should work

vtjnash · 2023-12-02T22:15:21Z

src/task.c

@@ -510,6 +510,8 @@ JL_NO_ASAN static void ctx_switch(jl_task_t *lastt)
    jl_set_pgcstack(&t->gcstack);
    jl_signal_fence();
    lastt->ptls = NULL;
+    fegetenv(&lastt->fenv);


This should come before the task switch, as some task switch implementations will clobber it

Where specifically should it go?

Or rather, it looks like the fesetenv is the one that needed to move

Where should it go?

Feel free to push to this branch

Already done. I don't know about the propagating behavior though. I think we might want to start every task with a call to set fesetenv(FE_DFL_ENV)?

vchuravy

I am still not convinced that this is a good thing to do. We have not yet specified what fenv means on the language level and if we want to support it in these cases. Echoing #51277 (comment)

Doesn't longjmp/setjmp save fenv? E.g. why do we have to do manually?

vtjnash · 2024-02-01T22:35:10Z

I don't think so. That is a very significant problem for anyone using this API, but I don't think it means it is wrong to do this PR (e.g. https://github.com/lattera/glibc/blob/master/sysdeps/x86_64/jmpbuf-offsets.h and https://github.com/lattera/glibc/blob/master/sysdeps/x86_64/__longjmp.S have no space to be capable of storing it)

vchuravy · 2024-02-02T01:41:13Z

Two notes:

The C standard defines fenv as thread-local, I do agree that it would make sense to define it as a scoped value (as implemented here) and a task would copy the fence from its parent.

I do still maintain that doing so is preemptive without adding language level support for strictfp and constrained floating point ops as defined by LLVM for this purpose.

Also see https://internals.rust-lang.org/t/equivalent-of-pragma-stdc-fenv-access-on/14934

I am okay with merging this partial support under the understanding that there is a declaration of intent to actually develop the full language semantics and that setting or accessing fenv remains UB.

Because right now messing with fenv and then entering the compiler that does some constant evaluation sounds like a recipe for causing hard to track down bugs.

Fixes #51277

might not be good to do, but it at least isn't worse than what we already do

oscardssmith · 2024-10-16T21:14:33Z

might not be good to do, but it at least isn't worse than what we already do

I disagree with this. This has a real (if small) costs and unclear benefit.

vtjnash · 2024-10-16T21:23:50Z

Fair: I didn't expect AMD to seem so bad at this.

  CPU: 128 × AMD EPYC 7513 32-Core Processor
julia> @btime Base.Rounding.setrounding_raw(Float64, Base.Rounding.rounding_raw(Float64))
  19.679 ns (0 allocations: 0 bytes)

julia> t = @eval @task while true; yieldto($(current_task())); end
Task (runnable) @0x00007fae5e5b1090

julia> @btime yieldto(t)
  134.496 ns (0 allocations: 0 bytes)

  CPU: 10 × Apple M2 Pro
julia> @btime Base.Rounding.setrounding_raw(Float64, Base.Rounding.rounding_raw(Float64))
  5.916 ns (0 allocations: 0 bytes)
0

julia> @btime yieldto(t)
  67.197 ns (0 allocations: 0 bytes)

  CPU: 8 × Intel(R) Core(TM) i7-8809G CPU @ 3.10GHz
julia> @btime Base.Rounding.setrounding_raw(Float64, Base.Rounding.rounding_raw(Float64))
  6.380 ns (0 allocations: 0 bytes)
0

julia> @btime yieldto(t)
  175.516 ns (0 allocations: 0 bytes)

@threads

Fixes #51277, though we give no guarantee that it keeps working this way, or that calling `setrounding_raw` won't lead to other undefined behavior. To give some examples: ```julia julia> t = Base.Rounding.setrounding_raw(Float64, Base.Rounding.to_fenv(RoundDown)) do Task(() -> println(rounding(Float64))) end Task (runnable) @0x000000010dff04c0 julia> rounding(Float64) RoundingMode{:Nearest}() julia> wait(schedule(t)) RoundingMode{:Down}() # currently gives RoundingMode{:Nearest}() julia> rounding(Float64) RoundingMode{:Nearest}() julia> Base.Rounding.setrounding_raw(Float64, Base.Rounding.to_fenv(RoundDown)) do Threads.@threads :static for i = 1:Threads.nthreads() println(Threads.threadid() => rounding(Float64)) end end 1 => RoundingMode{:Down}() 2 => RoundingMode{:Down}() # currently gives RoundingMode{:Nearest}() 4 => RoundingMode{:Down}() # currently gives RoundingMode{:Nearest}() 3 => RoundingMode{:Down}() # currently gives RoundingMode{:Nearest}() ```

JeffBezanson · 2024-12-05T22:51:26Z

For me this more than doubled task switch time, from 120ns to 290ns. This at least needs to be discussed.

This reverts commit 12aa9de.

This reverts commit 12aa9de. un-fixes #51277 Alternatively, we could add a flag for whether the state is non-default so we know whether `fesetenv` is necessary, if this behavior is considered desireable.

brenhinkeller added maths Mathematical functions multithreading Base.Threads and related functionality labels Sep 14, 2023

simonbyrne force-pushed the sb/task-fenv branch from 64aa1de to dbafc69 Compare December 2, 2023 19:32

vtjnash approved these changes Dec 2, 2023

View reviewed changes

vtjnash requested a review from vchuravy February 1, 2024 16:05

vchuravy previously requested changes Feb 1, 2024

View reviewed changes

simonbyrne and others added 2 commits February 26, 2024 21:48

add fenv cache to task struct

51429b6

Fixes #51277

Update task.c

eb91a2e

simonbyrne force-pushed the sb/task-fenv branch from 147e605 to eb91a2e Compare February 27, 2024 05:48

Merge branch 'master' into sb/task-fenv

4405cf1

simonbyrne marked this pull request as ready for review October 16, 2024 05:43

vtjnash merged commit 12aa9de into master Oct 16, 2024
6 of 8 checks passed

vtjnash deleted the sb/task-fenv branch October 16, 2024 20:54

JeffBezanson added the triage This should be discussed on a triage call label Dec 5, 2024

JeffBezanson added a commit that referenced this pull request Jan 7, 2025

Revert "add fenv cache to task struct (#51288)"

d0fd51d

This reverts commit 12aa9de.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add fenv cache to task struct #51288

add fenv cache to task struct #51288

simonbyrne commented Sep 13, 2023

vtjnash left a comment

vtjnash Dec 2, 2023

simonbyrne Dec 2, 2023

vtjnash Feb 1, 2024

simonbyrne Feb 1, 2024

simonbyrne Feb 1, 2024

vtjnash Feb 1, 2024

vchuravy left a comment

vtjnash commented Feb 1, 2024

vchuravy commented Feb 2, 2024

oscardssmith commented Oct 16, 2024

vtjnash commented Oct 16, 2024

JeffBezanson commented Dec 5, 2024

add fenv cache to task struct #51288

add fenv cache to task struct #51288

Conversation

simonbyrne commented Sep 13, 2023

vtjnash left a comment

Choose a reason for hiding this comment

vtjnash Dec 2, 2023

Choose a reason for hiding this comment

simonbyrne Dec 2, 2023

Choose a reason for hiding this comment

vtjnash Feb 1, 2024

Choose a reason for hiding this comment

simonbyrne Feb 1, 2024

Choose a reason for hiding this comment

simonbyrne Feb 1, 2024

Choose a reason for hiding this comment

vtjnash Feb 1, 2024

Choose a reason for hiding this comment

vchuravy left a comment

Choose a reason for hiding this comment

vtjnash commented Feb 1, 2024

vchuravy commented Feb 2, 2024

oscardssmith commented Oct 16, 2024

vtjnash commented Oct 16, 2024

JeffBezanson commented Dec 5, 2024