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

Make @fastmath aware of Base.literal_pow #21099

Closed
wants to merge 1 commit into from

Conversation

ajkeller34
Copy link
Contributor

as Base.literal_pow(Base.FastMath.pow_fast, x, Val{p}).

Thanks to PRs #20530 and #20889, exponentiation of a dimensionful quantity can be type-stable in some important circumstances. However, because macro expansion occurs before the Val lowering happens, @fastmath x^2 cannot be type-stable for dimensionful quantities, whereas x^2 could be. This is because @fastmath x^2 is macro-expanded to Base.FastMath.pow_fast(x,2) and so we can never catch the ^ symbol. See the tests I added to get an idea of how this works.

Provided the premise of this PR isn't flawed somehow, it would probably good to have someone more familiar with julia_syntax.scm see if there’s a better way to implement this change. Because I tie into the mechanism introduced in #20889 I don't expect any "surprising behaviors" to be introduced here, though I haven't checked to see if there are any performance implications.

@ararslan ararslan added compiler:lowering Syntax lowering (compiler front end, 2nd stage) domain:maths Mathematical functions labels Mar 20, 2017
@tkelman
Copy link
Contributor

tkelman commented Mar 20, 2017

dunno if nanosoldier will really notice lowering performance, but may as well trigger @nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@TotalVerb
Copy link
Contributor

It would make more sense to do this in the @fastmath macro than in lowering.

@StefanKarpinski
Copy link
Sponsor Member

It would make more sense to do this in the @fastmath macro than in lowering.

100% – we can't go around having special cases for every macro anyone might need special behavior for in the parser.

@ajkeller34
Copy link
Contributor Author

Yeah, good point, I'll see what I can do.

@ajkeller34
Copy link
Contributor Author

Updated, this approach seems much better and obvious in retrospect. I guess this is what happens when you learn about how lisp works for the first time and get a little too exuberant.

@ajkeller34 ajkeller34 changed the title RFC: Lower Base.FastMath.pow_fast(x,p) for integer literal p... RFC: Make @fastmath aware of Base.literal_pow Mar 20, 2017
@KristofferC
Copy link
Sponsor Member

Let's rerun CI and Nanosoldier and try get this merged.

@KristofferC KristofferC reopened this Jul 21, 2017
@KristofferC
Copy link
Sponsor Member

@nanosoldier runbenchmarks(ALL, vs = ":master")

@stevengj
Copy link
Member

It would be nice to have @fastmath convert x^literal to an optimal chain of multiplications ala #20637, but we can leave that for a future PR.

@nanosoldier
Copy link
Collaborator

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @ararslan

@KristofferC
Copy link
Sponsor Member

Tests seems to fail now. Is it because the Val optimization is only made to "hardware numbertypes" now?

@ajkeller34
Copy link
Contributor Author

I updated the PR, squashed, and rebased. fastmath tests passed locally. The problem was because the PR hadn't adopted the new Val(x) syntax from #22475.

@ajkeller34
Copy link
Contributor Author

@KristofferC want to try triggering nanosoldier again? Looks like the tests passed (travis timed out for likely unrelated reasons).

@ararslan
Copy link
Member

ararslan commented Jul 29, 2017

Another Nanosoldier run is unlikely to work, Nanosoldier itself is still having problems (unrelated to this PR)

@KristofferC
Copy link
Sponsor Member

I thought it worked a few times yesterday @nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@ararslan
Copy link
Member

It's been working randomly but failing most of the time

@ajkeller34 ajkeller34 changed the title RFC: Make @fastmath aware of Base.literal_pow Make @fastmath aware of Base.literal_pow Aug 1, 2017
@ajkeller34
Copy link
Contributor Author

This is ready to merge as far as I can tell. Does anything else need to be done here?

test/fastmath.jl Show resolved Hide resolved
@JeffBezanson JeffBezanson added this to the 1.0.x milestone Apr 19, 2018
@stevengj
Copy link
Member

stevengj commented Jan 4, 2019

Bump.

@stevengj
Copy link
Member

stevengj commented Apr 5, 2020

Any chance of merging this?

@StefanKarpinski StefanKarpinski added the status:triage This should be discussed on a triage call label Apr 5, 2020
@JeffBezanson
Copy link
Sponsor Member

From triage: seems fine, just rebase please.

@stevengj
Copy link
Member

stevengj commented Jan 7, 2021

Rebased.

@oscardssmith
Copy link
Member

The test this PR adds is the one that's failing.

@stevengj
Copy link
Member

stevengj commented Jan 8, 2021

Interacting badly with #24240, will fix.

@stevengj
Copy link
Member

stevengj commented Jan 8, 2021

Actually, #24240 already seems to fix this issue in another way — @fastmath x^2 goes to pow_fast(x, Val{2}()) which calls literal_pow by default.

So maybe this can be closed?

@oscardssmith oscardssmith removed the status:triage This should be discussed on a triage call label Jan 8, 2021
@vtjnash vtjnash closed this Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:lowering Syntax lowering (compiler front end, 2nd stage) domain:maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet