/purescript

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

Optimise (f <<< g $ x) as (f (g x)) #1056

Merged
merged 1 commit into from Apr 18, 2015

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@puffnfresh @paf31 @garyb
@puffnfresh
Contributor

puffnfresh commented Apr 18, 2015

When writing PureScript code like:

not <<< not $ id true

We see a few function calls:

Prelude["<<<"](Prelude.semigroupoidArr)
    (Prelude.not(Prelude.boolLikeBoolean))
    (Prelude.not(Prelude.boolLikeBoolean))
    (Prelude.id(Prelude.categoryArr)(
        true));

We now see something very straight forward:

!!Prelude.id(Prelude.categoryArr)(true);

The implementation of this optimisation contains a lot of similarities
to the common binary operator inlining. We should extract some
functions out in another commit.
@puffnfresh
Optimise (f <<< g $ x) as (f (g x)) 
When writing PureScript code like:

    not <<< not $ id true

We see a few function calls:

    Prelude["<<<"](Prelude.semigroupoidArr)
        (Prelude.not(Prelude.boolLikeBoolean))
        (Prelude.not(Prelude.boolLikeBoolean))
        (Prelude.id(Prelude.categoryArr)(
            true));

We now see something very straight forward:

    !!Prelude.id(Prelude.categoryArr)(true);

The implementation of this optimisation contains a lot of similarities
to the common binary operator inlining. We should extract some
functions out in another commit.
d8d96cf
@puffnfresh

This comment has been minimized.

Show comment
Hide comment
@puffnfresh

puffnfresh Apr 18, 2015

Contributor

Hacked this up pretty quick and it's getting late. Haven't even run tests yet, just putting my work somewhere to track early.
Contributor

puffnfresh commented Apr 18, 2015

Hacked this up pretty quick and it's getting late. Haven't even run tests yet, just putting my work somewhere to track early.
src/Language/PureScript/CodeGen/JS/Optimizer.hs
@@ -77,7 +77,8 @@ optimize' js = do
, inlineOperator (C.prelude, (C.$)) $ \f x -> JSApp f [x]
, inlineOperator (C.prelude, (C.#)) $ \x f -> JSApp f [x]
, inlineOperator (C.preludeUnsafe, C.unsafeIndex) $ flip JSIndexer
, inlineCommonOperators ]) js
, inlineCommonOperators
, inlineAppliedArrComposition ]) js

This comment has been minimized.

Show comment
Hide comment
@puffnfresh

puffnfresh Apr 18, 2015

Contributor

Would this be the right place to put this optimisation? Is there a better order?
Copy URL
@puffnfresh

puffnfresh Apr 18, 2015

Contributor

Would this be the right place to put this optimisation? Is there a better order?

This comment has been minimized.

Show comment
Hide comment
@paf31

paf31 Apr 18, 2015

Member

Finding the best order is tricky in general. I've done it by trial and error in the past, but usually I have issues with things like runST and do notation. I don't think there's any issue here though.
Copy URL
@paf31

paf31 Apr 18, 2015

Member

Finding the best order is tricky in general. I've done it by trial and error in the past, but usually I have issues with things like runST and do notation. I don't think there's any issue here though.
src/Language/PureScript/CodeGen/JS/Optimizer/Inliner.hs
@@ -31,6 +32,10 @@ import Language.PureScript.Names
import Language.PureScript.CodeGen.JS.Optimizer.Common
import qualified Language.PureScript.Constants as C
-- TODO: Potential bug:

This comment has been minimized.

Show comment
Hide comment
@paf31

paf31 Apr 18, 2015

Member

I didn't realize 0..toFixed was even valid syntax ... interesting :)
Copy URL
@paf31

paf31 Apr 18, 2015

Member

I didn't realize 0..toFixed was even valid syntax ... interesting :)
@paf31

This comment has been minimized.

Show comment
Hide comment
@paf31

paf31 Apr 18, 2015

Member

Looks great. I was thinking about this today too funnily enough. I should be able to merge this tomorrow.
Member

paf31 commented Apr 18, 2015

Looks great. I was thinking about this today too funnily enough. I should be able to merge this tomorrow.
@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Apr 18, 2015

Member

Do we inline f $ x at the moment? That case is worth covering too I'd think. Both of these things I'd intended to add in a CoreFn optimizer step, but we don't have one of those yet.
Member

garyb commented Apr 18, 2015

Do we inline f $ x at the moment? That case is worth covering too I'd think. Both of these things I'd intended to add in a CoreFn optimizer step, but we don't have one of those yet.
@puffnfresh

This comment has been minimized.

Show comment
Hide comment
@puffnfresh

puffnfresh Apr 18, 2015

Contributor

@garyb yeah, we do $ already. This optimisation even relies upon that step.
Contributor

puffnfresh commented Apr 18, 2015

@garyb yeah, we do $ already. This optimisation even relies upon that step.

paf31 added a commit that referenced this pull request Apr 18, 2015

@paf31
Merge pull request #1056 from puffnfresh/optimisation/elim-function-s… 
…emigroupoid

Optimise (f <<< g $ x) as (f (g x))
c892a26

@paf31 paf31 merged commit c892a26 into purescript:master Apr 18, 2015
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@puffnfresh puffnfresh referenced this pull request Apr 20, 2015

Closed

Optimisation: (f <<< g) ~ function(_0) { return f(g(_0)); } #1066

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