Skip to content

Move inline β-reduction out of typer #5216

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

Merged
merged 17 commits into from
Oct 22, 2018

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Oct 8, 2018

Move inlining after Pickler into InlineCalls phase which only runs if there is something to inline.

@nicolasstucki nicolasstucki self-assigned this Oct 8, 2018
The issue is that we created bindings of the form `val xyz_this: => XYZ = ...`
@nicolasstucki nicolasstucki force-pushed the move-inline-out-of-typer branch from 246df88 to 4b9cc39 Compare October 8, 2018 15:41
@nicolasstucki nicolasstucki force-pushed the move-inline-out-of-typer branch from 4b9cc39 to 432eb0a Compare October 8, 2018 16:37
@nicolasstucki nicolasstucki force-pushed the move-inline-out-of-typer branch from 7ff4e80 to c68dc1f Compare October 8, 2018 18:46
@nicolasstucki nicolasstucki force-pushed the move-inline-out-of-typer branch from 63c8038 to 32c8798 Compare October 8, 2018 19:19
@nicolasstucki nicolasstucki force-pushed the move-inline-out-of-typer branch 3 times, most recently from 3c9f76c to 8543a33 Compare October 8, 2018 20:49
@allanrenucci allanrenucci mentioned this pull request Oct 9, 2018
8 tasks
@nicolasstucki nicolasstucki force-pushed the move-inline-out-of-typer branch 2 times, most recently from 456f075 to db7e183 Compare October 9, 2018 07:22
@nicolasstucki
Copy link
Contributor Author

test performance please

@dottybot
Copy link
Member

dottybot commented Oct 9, 2018

performance test scheduled: 1 job(s) in queue, 0 running.

@dottybot
Copy link
Member

dottybot commented Oct 9, 2018

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/5216/ to see the changes.

Benchmarks is based on merging with master (7463afe)

@nicolasstucki nicolasstucki force-pushed the move-inline-out-of-typer branch from db7e183 to 927ae4e Compare October 9, 2018 14:09
When inlining a `this` binding that referes to a result typed
with a match type, this type must be normalized and used.
@nicolasstucki nicolasstucki force-pushed the move-inline-out-of-typer branch from 0101eb6 to c6c5389 Compare October 9, 2018 15:26
Previously the `implicitly[C]` was refining its return type of `ev` by inlining.
As we are now inlining after typer we refine the return type explicitly to `ev.type`.
@nicolasstucki nicolasstucki force-pushed the move-inline-out-of-typer branch from 4c4733c to 0832e02 Compare October 9, 2018 17:18
@nicolasstucki
Copy link
Contributor Author

test performance please

@dottybot
Copy link
Member

dottybot commented Oct 9, 2018

performance test scheduled: 1 job(s) in queue, 0 running.

@dottybot
Copy link
Member

dottybot commented Oct 9, 2018

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/5216/ to see the changes.

Benchmarks is based on merging with master (7a45a4a)

@nicolasstucki nicolasstucki force-pushed the move-inline-out-of-typer branch from 6eebf53 to 894ecee Compare October 16, 2018 14:56
Copy link
Contributor

@Duhemm Duhemm left a comment

Choose a reason for hiding this comment

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

If no Inlined nodes can appear before InlineCalls, I think that you can also remove some cases from the sbt phases.

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

I would argue that the rhs of erased types should be erased in PostTyper (and don't need an inline expansion either). Otherwise LGTM.

/** Transforms the rhs tree into a its default tree if it is in an `erased` val/def.
* Performed to shrink the tree that is known to be erased later.
*/
private def normalizeErasedRhs(rhs: Tree, sym: Symbol)(implicit ctx: Context) =
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens to erased right hand sides now? Are they kept? Why can't they be dropped here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The right-hand sides are kept as they were before. Implicit arguments to erased methods are removed early (as (???: T)).

This logic was only droping nodes that where inlined in an erased rhs, the rest was unaffected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to erase the rhs of erased val/def that are not inline. But YCheck complained on most erased tests. I will investigate further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I managed to implement it, now rhs of erased non inlined methods are removed in PostTyper

@odersky odersky assigned nicolasstucki and unassigned odersky Oct 20, 2018
@nicolasstucki nicolasstucki force-pushed the move-inline-out-of-typer branch 2 times, most recently from 8d3c7a2 to 444943a Compare October 20, 2018 11:43
@nicolasstucki nicolasstucki force-pushed the move-inline-out-of-typer branch from 6d9474b to c12e875 Compare October 22, 2018 06:00
@nicolasstucki
Copy link
Contributor Author

@odersky the last commit erases the RHS of erased def/val in PostTyper.

@odersky odersky merged commit 7067122 into scala:master Oct 22, 2018
@allanrenucci allanrenucci deleted the move-inline-out-of-typer branch October 22, 2018 15:03
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.

4 participants