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

Lift decls js backend #3821

Merged
merged 27 commits into from
May 23, 2017
Merged

Conversation

rbarreiro
Copy link
Collaborator

@rbarreiro rbarreiro commented May 16, 2017

I have made a new js backend based on the liftDecls.

It is passing all the tests. And works much better with foreign functions that take functions as arguments,
This test https:/rbarreiro/idrisjs/blob/master/tests/t8.idr is not working as expected with the current backend.

I also prefer the javascript generated directly from lifstDecls to the one produced by the current backend.

Can you please review it?

@melted melted self-assigned this May 16, 2017
@david-christiansen
Copy link
Contributor

Nifty! A couple of questions, as I don't have time to dig into code just right now:

  1. Are you planning on maintaining this on an ongoing basis?
  2. What do you see as the major design trade-offs where this backend differs from the current one?
  3. How do you handle tail call elimination?
  4. Is this backwards-compatible with e.g. FFI code written in the previous backend?

@melted
Copy link
Contributor

melted commented May 16, 2017

Thanks for the PR!

I will start a review, it's going to take a while. Others shouldn't hesitate to take a look too :-)

@rbarreiro
Copy link
Collaborator Author

  1. Yes
  2. The only thing worst I can think of is that width this backend we can have stackoverflows for mutual recursive tail calls. It generates the code faster, less awfull javascript generated(subjective), smaller after closure compiler, working with ffi involving lambdas and I did not benchmark it, but I believe it runs faster.
  3. Replace self tail calls width loop
  4. I tried and I believe i succeed.

@melted
Copy link
Contributor

melted commented May 16, 2017

Can you add a commit that updates the CHANGELOG?

@melted
Copy link
Contributor

melted commented May 16, 2017

I looked at the Travis outcomes:

  • Stylize failed. I ran stylize.sh and it rearranged the imports a bit. So just install stylish-haskell and run that and commit.
  • A bigger deal is that GHC 7.6 and 7.8 builds failed because they don't support DeriveAnyClass.
  • The javascript tests failed, as far as I can see just because node screamed about 'Use of const in strict mode'. Likely Travis comes with an old node.

mapMapListKeys f (t:r) x = mapMapListKeys f r $ Map.adjust f t x


extract_globs :: Map Name LDecl -> LDecl -> [Name]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to camelcase, here and the other underscored names in the file

}


get_include :: FilePath -> IO Text
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change these names to camelcase

, "}\n\n"
]

js_aux_defs = T.concat [throw2, force]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to camelcase

@rbarreiro
Copy link
Collaborator Author

I have pushed some changes to address these problems. Regarding the CHANGELOG, I need to create a new Header "New in ...", right? What version?

@melted
Copy link
Contributor

melted commented May 17, 2017 via email

@melted
Copy link
Contributor

melted commented May 22, 2017

I'll merge this tomorrow unless there are objections.

@edwinb any comments?

@melted melted merged commit a859f65 into idris-lang:master May 23, 2017
@melted
Copy link
Contributor

melted commented May 23, 2017

Thanks for the contribution!

@ahmadsalim
Copy link

This is great!

@be5invis I think I remember you may have had some custom optimizations for a version of this backend. Could you please consider backporting the improvements here?

@be5invis
Copy link
Contributor

@ahmadsalim There are some, like uncurrying, data type specialization, custom rewriting rules, etc.
https:/be5invis/idris-codegen-es

@be5invis
Copy link
Contributor

be5invis commented May 23, 2017

@ahmadsalim The differences are:

  • Idris-codegen-es has data type specialization, that maps Idris’ data types into JavaScript equivalent.
  • Idris-codegen-es uses If tree to generate Cases, while @rbarreiro’s uses Switch statement. Using Switch would be problematic when you want to specialize data types (like turning Idris True/False into JavaScript true/false)
  • Idris-codegen-es has special rewriting rules to eliminate silly overheads caused by data types.
  • Idris-codegen-es targets ES6...

@ahmadsalim
Copy link

@be5invis That sounds great!
Would it be possible for you to try and synchronize these changes with the new backend proposed here? (E.g. by making a new PR that adds your changes?). I think it would be very useful to have these optimizations!

@be5invis
Copy link
Contributor

@ahmadsalim I'd like to open another PR.

@ahmadsalim
Copy link

@be5invis Yeah, that is what I meant :). But I think there has been some updates to this backend that fixes some issues, and it would be nice if your work incorporated the changes.

@be5invis
Copy link
Contributor

@ahmadsalim The compiler part and the FFI/primitive part are, well, pretty separated.
My modifications are mainly on the compiler part -- making the code faster, more beautiful, etc.

@rbarreiro rbarreiro deleted the liftDecls_js_backend branch May 23, 2017 08:50
@ahmadsalim
Copy link

@be5invis I see, in any case you are welcome to submit a new PR.

@be5invis
Copy link
Contributor

@ahmadsalim I'd like to add the ES codegen entirely into Idris first and move the FFI/primitive thing into ES...
Idris-codegen-ES changed A LOT. REALLY, A LOT.

@ahmadsalim
Copy link

@be5invis I see, I was not aware on the degree things changed.

I think a suggestion could be that you fork the latest version of Idris, add your changes there, move the FFI/primitives and then make the PR if possible. Do you agree?

@be5invis
Copy link
Contributor

@ahmadsalim I'll do that.

@be5invis
Copy link
Contributor

@ahmadsalim The current version already has uncurry (though... looks not beautiful 😞)
I'd like to do those:

  • Separate JsAST into JsExpr and JsStmt
  • Data type specialization
  • Rewriting

@comonoid
Copy link

comonoid commented May 23, 2017

Idris-codegen-es targets ES6...

Good news?... Maybe...
Sorry for my dumb question, but...
Would you please tell me where can i find something
about browser compatibility with this ES6 subset?

@be5invis
Copy link
Contributor

@rbarreiro
Copy link
Collaborator Author

Regarding ES6 in practice, depending on the features the closure compiler can convert ES6 to ES5. But you might have some trouble with the CI. I was using "const" and it failed the tests.

@ahmadsalim
Copy link

@be5invis I think the optimizations sound good.
Perhaps, you can coordinate with @rbarreiro about the changes in code organization (refactoring), since I am not completely into the details.

@comonoid
Copy link

Also, i was remembered about babel.
So, as i can see now, generally, ES6 is not a problem now ;-)
Thanks!

@rbarreiro
Copy link
Collaborator Author

rbarreiro commented May 23, 2017

@be5invis I gave a quick look to repo and I liked what you did in JsAST.hs, JsName.hs

Regarding the rewriting, do you think you can do it creating less complexity on the cgBody function, maybe using uniplate and some replace rule with LForeign, what do you think? If it is not possible we can discuss further.

I would not like to make cgBody more complex, it is already a bit hard to read. What do you think?

@rbarreiro
Copy link
Collaborator Author

I also prefer your solution to uncurry.

@rbarreiro
Copy link
Collaborator Author

Just one question, I moved the lambdas to the partial application functions, because in-line it raised problems with the tail call optim, I was creating new lambdas using the variables in the loop, so I was changing the lambdas by changing the variable in the loop. How did you addressed this?

@rbarreiro
Copy link
Collaborator Author

@be5invis One more question, you changed the all uses of javascript switch statement to if?
Why?

@be5invis
Copy link
Contributor

@rbarreiro For data type specialization.
In idris-codegen-es, nullary data construction would be specialized into a number, and built-in types like True and False would be mapped into JavaScript true and false. So, yes, it need to be changed into if.

@rbarreiro
Copy link
Collaborator Author

rbarreiro commented May 23, 2017 via email

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.

6 participants