Skip to content

Inliner improvements #6218

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
Apr 14, 2019
Merged

Inliner improvements #6218

merged 17 commits into from
Apr 14, 2019

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Apr 3, 2019

This is some preliminary work to improve the Inliner to allow a larger set of idioms to be used in typeclass derivation. The idioms are tried in typeclass-derivation2{b|c}.scala, which serve as test-beds for now.

odersky added 6 commits April 3, 2019 11:20
No need to enter new bindings into a context, since all references
are symbolic.
Avoids clutter in traces if staging is not relevant
 - Don't destructively update the symbol of a case binding.
   This does not work reliably as the old info may flow
   into cached types.
 - Instead, create new case binding symbols and substitute old for new.
Need to take current context instead of global Inliner context.
@odersky odersky marked this pull request as ready for review April 3, 2019 21:14
The previous treatment could go wrong by confusing `bindings` and `termBindings`
in a recursive step.

test case: run/typeclass-derivation2c.scala
odersky added 4 commits April 4, 2019 18:08
This leads to better generated code. Downside is a slight increase in the
code of the deriving typeclasses.
odersky added 5 commits April 5, 2019 10:18
ValueOf would happily return a value for non-value types, e.g. valueOf[String].
This would crash the backend later.

This commit replaces the crash with a type error. Ideally, though, this should
work as expected in inlined code.
@odersky
Copy link
Contributor Author

odersky commented Apr 5, 2019

run/typeclass-derivation2c.scala is a worked out strawman that illustrates a feasible code generation strategy for typeclass derivation. It would be good to have the input of everyone involved before potentially proceeding to an implementation of these generation patterns.

For more details, see: #6153 (comment)

@odersky
Copy link
Contributor Author

odersky commented Apr 5, 2019

This needs a technical review whether the bug fixes and improvements in the inliner make sense. For the purpose of this review we can treat the new typeclass-derivation{a|b}.scala files simply as tests.

We should separately discuss whether this is the scheme that should be implemented by the compiler. I propose we do that second discussion in #6153.

I believe it's best to do the review by commit.

@milessabin
Copy link
Contributor

@odersky I'm having trouble figuring out what the bugfixes and/or inliner improvements actually are here. Can you briefly summarise?

@odersky
Copy link
Contributor Author

odersky commented Apr 8, 2019

The compiler changes are in the commit messages of e2e56eb, b939933, b0b2383, f7d2658, 20e54a3.
The other commits elaborate the test cases.

@milessabin milessabin merged commit 25a3773 into scala:master Apr 14, 2019
@smarter
Copy link
Member

smarter commented Apr 14, 2019

Merging this PR broke the CI: http://dotty-ci.epfl.ch/lampepfl/dotty/12073/4

tests/neg/i6241.scala failed

@allanrenucci allanrenucci deleted the inliner-fixes branch April 15, 2019 02:45
@milessabin
Copy link
Contributor

Ahh, right ... that became a pos test.

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