Skip to content

minor improvements for pass pipeline#1424

Merged
dcodeIO merged 14 commits into
AssemblyScript:masterfrom
MaxGraey:pipeline
Aug 5, 2020
Merged

minor improvements for pass pipeline#1424
dcodeIO merged 14 commits into
AssemblyScript:masterfrom
MaxGraey:pipeline

Conversation

@MaxGraey

@MaxGraey MaxGraey commented Aug 3, 2020

Copy link
Copy Markdown
Member

Add more pre-SSA passes like simplify-locals-notee-nostructure and move redundant set elimination before SSA.

  • I've read the contributing guidelines

@MaxGraey MaxGraey requested a review from dcodeIO August 3, 2020 06:50
@MaxGraey MaxGraey changed the title Slightly improve pass pipeline minor improvements for pass pipeline Aug 3, 2020
@dcodeIO

dcodeIO commented Aug 3, 2020

Copy link
Copy Markdown
Member

I worry a bit that we are adding so much passes to the pipeline (removing 4, adding 9, partly conditional) that optimization will become slower and slower while we keep doing this. I see that the additions reduce fixtures by about 330 lines, which is good in classical terms, and I'd lean towards merging, yet the relativistic concern remains that achieving the speed of light (zero lines of output) can only be achieved by supplying infinite energy (running passes forever) and is ultimately impossible because of diminishing returns unless becoming massless (optimizing no code at all).

Or: Where do we draw the line? How much slowdown are we willing to accept for what gain? And how can we measure this as a function of time taken vs code optimized?

@MaxGraey

MaxGraey commented Aug 3, 2020

Copy link
Copy Markdown
Member Author

Basically this attempts also speedup pipeline by doing some work earlier, before SSA and flattering. In my internal tests it's really little bit faster than previous scheme.

Where do we draw the line? How much slowdown are we willing to accept for what gain? And how can we measure this as a function of time taken vs code optimized?

It will be great have some compiler benchmark with aggregate perf metrics and ideally with CI integration

@MaxGraey

MaxGraey commented Aug 3, 2020

Copy link
Copy Markdown
Member Author

For npm run test:compiler -- --create (and via ts-node) roughly comparison:
master: Time 80896 ms
this pr: Time 77528 ms

@dcodeIO dcodeIO left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In this case, LGTM :)

@dcodeIO

dcodeIO commented Aug 5, 2020

Copy link
Copy Markdown
Member

Since this touches a lot of fixtures, can you merge master into this PR and regenerate the fixtures? Mostly just in case, i.e. if an export not present before #1422 would now be present and somehow introduce a fixture-only conflict.

@MaxGraey

MaxGraey commented Aug 5, 2020

Copy link
Copy Markdown
Member Author

Sure, done! But it seems it hasn't any changes

@dcodeIO dcodeIO merged commit 176e932 into AssemblyScript:master Aug 5, 2020
@dcodeIO

dcodeIO commented Aug 5, 2020

Copy link
Copy Markdown
Member

Thanks!

@MaxGraey MaxGraey deleted the pipeline branch August 5, 2020 18:28
@github-actions

github-actions Bot commented Aug 6, 2020

Copy link
Copy Markdown

🎉 This PR is included in version 0.14.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants