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

improve concrete-foldability of core math functions #45613

Merged
merged 3 commits into from
Jun 11, 2022
Merged

Conversation

aviatesk
Copy link
Sponsor Member

@aviatesk aviatesk commented Jun 8, 2022

By making more use of the @assume_effects annotation. This commit adds
several :consistent annotations to certain functions that access to
constant global table (e.g. INV_2PI and J_TABLE), because currently
our effect analysis can't prove the consistency in the presence of
@inbounds and :boundscheck.

In the long term, we can revert this commit once we improve the effect
analysis so that it can properly reason about safe tuple indexing.


  • these commits should be merged separately
  • @nanosoldier runbenchmarks(ALL, vs=":master")

@oscardssmith
Copy link
Member

This looks good to me.

@nanosoldier
Copy link
Collaborator

Something went wrong when running your job:

NanosoldierError: error when preparing/pushing to report repo: failed process: Process(setenv(`git push`; dir="/nanosoldier/workdir/NanosoldierReports"), ProcessExited(1)) [1]

Unfortunately, the logs could not be uploaded.

By making more use of the `@assume_effects` annotation. This commit adds
several `:consistent` annotations to certain functions that access to
constant global table (e.g. `INV_2PI` and `J_TABLE`), because currently
our effect analysis  can't prove the consistency in the presence of
`@inbounds` and `:boundscheck`.

In the long term, we can revert this commit once we improve the effect
analysis so that it can properly reason about safe tuple indexing.
@aviatesk
Copy link
Sponsor Member Author

aviatesk commented Jun 9, 2022

@Keno could I hear your thought on this? Especially I'm wondering about:

because currently our effect analysis can't prove the consistency in the presence of @inbounds and :boundscheck.

In the long term, we can revert this commit once we improve the effect analysis so that it can properly reason about safe tuple indexing.

@aviatesk
Copy link
Sponsor Member Author

Seems like some of scalar benchmarks get improved by these annotations.

@aviatesk aviatesk merged commit a3783c7 into master Jun 11, 2022
@aviatesk aviatesk deleted the avi/concretemath branch June 11, 2022 08:36
@vtjnash
Copy link
Sponsor Member

vtjnash commented Jun 11, 2022

win32 bots appear to be mad at this PR

      From worker 6:	┌ Error: bad effects found for sin(::Float32)
      From worker 6:	│   eff = (+c,+e,?n,?t,+s)
      From worker 6:	└ @ Main.Test85Main_math C:\buildbot\worker-tabularasa\tester_win32\build\share\julia\test\math.jl:1464
      From worker 6:	┌ Error: bad effects found for cos(::Float32)
      From worker 6:	│   eff = (+c,+e,?n,?t,+s)
      From worker 6:	└ @ Main.Test85Main_math C:\buildbot\worker-tabularasa\tester_win32\build\share\julia\test\math.jl:1464
      From worker 6:	┌ Error: bad effects found for tan(::Float32)
      From worker 6:	│   eff = (+c,+e,?n,?t,+s)
      From worker 6:	└ @ Main.Test85Main_math C:\buildbot\worker-tabularasa\tester_win32\build\share\julia\test\math.jl:1464
math                                     (6) |         failed at 2022-06-11T03:45:08.240
Test Failed at C:\buildbot\worker-tabularasa\tester_win32\build\share\julia\test\math.jl:1465
  Expression: false
Test Failed at C:\buildbot\worker-tabularasa\tester_win32\build\share\julia\test\math.jl:1465
  Expression: false
Test Failed at C:\buildbot\worker-tabularasa\tester_win32\build\share\julia\test\math.jl:1465
  Expression: false

@vchuravy
Copy link
Sponsor Member

@aviatesk
Copy link
Sponsor Member Author

These failures seem to happen non-deterministically... For now I'd like to just skip these tests on x86 machines.

aviatesk added a commit that referenced this pull request Jun 14, 2022
But just print bad effects instead – especially `[sin|cos|tan](::Float32)`
seem to be analyzed as non-foldable sometimes non-deterministically, somehow.
We need to dig into what's leading to the bad analysis with Cthulhu on
each platform, but this homework is left for the readers with access.
vtjnash pushed a commit that referenced this pull request Jun 15, 2022
But just print bad effects instead – especially `[sin|cos|tan](::Float32)`
seem to be analyzed as non-foldable sometimes non-deterministically, somehow.
We need to dig into what's leading to the bad analysis with Cthulhu on
each platform, but this homework is left for the readers with access.

Tests added in #45613
@KristofferC KristofferC added the backport 1.8 Change should be backported to release-1.8 label Jul 20, 2022
KristofferC pushed a commit that referenced this pull request Jul 20, 2022
But just print bad effects instead – especially `[sin|cos|tan](::Float32)`
seem to be analyzed as non-foldable sometimes non-deterministically, somehow.
We need to dig into what's leading to the bad analysis with Cthulhu on
each platform, but this homework is left for the readers with access.

Tests added in #45613

(cherry picked from commit ef42205)
@aviatesk aviatesk mentioned this pull request Jul 20, 2022
32 tasks
aviatesk added a commit that referenced this pull request Jul 26, 2022
improve concrete-foldability of core math functions
aviatesk added a commit that referenced this pull request Jul 26, 2022
But just print bad effects instead – especially `[sin|cos|tan](::Float32)`
seem to be analyzed as non-foldable sometimes non-deterministically, somehow.
We need to dig into what's leading to the bad analysis with Cthulhu on
each platform, but this homework is left for the readers with access.

Tests added in #45613
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Aug 7, 2022
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.

7 participants