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

feat(compiler)!: Enable tail calls by default #1589

Merged
merged 3 commits into from
Mar 11, 2023

Conversation

spotandjake
Copy link
Member

@spotandjake spotandjake commented Jan 9, 2023

This pr enables tail calls by default it adds the cli argument --no-wasm-tail-call and removes the --experimental-wasm-tail-calls cli argument.

This is marked as a draft because in #1587 it is noted that there are some cases where tail calls currently leak which need to be fixed before this is default.

closes: #1587

@spotandjake spotandjake force-pushed the spotandjake-default-tail-calls branch from b2868c2 to 63b97ee Compare January 9, 2023 21:40
@peblair
Copy link
Member

peblair commented Jan 11, 2023

@phated (apologies if this duplicates Discord discussion) What do you recommend we do about the NEAR smoketest here? Aside from the failing CI, this looks good.

@spotandjake
Copy link
Member Author

spotandjake commented Jan 11, 2023

I was thinking we could pass in the compiler flag to not use tail calls for those tests. But was wondering the same thing, should be as simple as changing /* grainc-flags --no-gc --no-bulk-memory */ to /* grainc-flags --no-gc --no-bulk-memory --no-wasm-tail-call*/ in the tests for near.

@spotandjake spotandjake force-pushed the spotandjake-default-tail-calls branch 2 times, most recently from b50569c to 23b74bf Compare January 11, 2023 23:43
@ospencer ospencer changed the title feat(compiler)!: Make Tail Calls Default feat(compiler)!: Replace --experimental-wasm-tail-call with --no-wasm-tail-call; enable tail calls by default Jan 13, 2023
@spotandjake spotandjake marked this pull request as ready for review February 22, 2023 02:51
@spotandjake
Copy link
Member Author

spotandjake commented Feb 22, 2023

@ospencer The gc3 test is failing with a `Maximum memory overflow.

image
I ran the test dissabling tail calls as well and it still failed. also We tried bumping the memory limit up to 2048 and it failed and then i tried 10048 and it still failed which isnt a good sign.

@phated phated force-pushed the spotandjake-default-tail-calls branch from 3a7f94d to afe0981 Compare March 7, 2023 04:04
@phated phated changed the title feat(compiler)!: Replace --experimental-wasm-tail-call with --no-wasm-tail-call; enable tail calls by default feat(compiler)!: Enable tail calls by default Mar 7, 2023
@ospencer ospencer added this pull request to the merge queue Mar 11, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 11, 2023
@ospencer ospencer force-pushed the spotandjake-default-tail-calls branch from afe0981 to 9a4aaf6 Compare March 11, 2023 21:38
@ospencer ospencer enabled auto-merge March 11, 2023 21:38
@ospencer ospencer added this pull request to the merge queue Mar 11, 2023
Merged via the queue into grain-lang:main with commit f6e5b00 Mar 11, 2023
av8ta pushed a commit to av8ta/grain that referenced this pull request Apr 11, 2023
* feat: Make Tail Calls Default

* fix: Disable Tail Calls In Near SmokeTest

* Update size of smallest grain program

---------

Co-authored-by: Oscar Spencer <oscar@grain-lang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Enable tail calls by default
3 participants