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

Optimize tailcalls even when called with different type arguments. #6065

Merged
merged 1 commit into from
Sep 28, 2017

Conversation

TomasMikula
Copy link
Contributor

This PR turns off the check that the recursive tail-call is made with the same type arguments, if the tailcall optimization is requested explicitly via @tailrec.

This allows to tail-optimize, for example, operations on type-aligned sequences.

I believe the check was there for a reason. However, the tests pass and I can't think of a case where it would be unsafe. Please let me know if I'm wrong.

@scala-jenkins scala-jenkins added this to the 2.12.4 milestone Sep 8, 2017
@TomasMikula TomasMikula changed the title Optimize tailcalls even called with different type arguments. Optimize tailcalls even when called with different type arguments. Sep 8, 2017
@sjrd
Copy link
Member

sjrd commented Sep 9, 2017

The check might have been there because it badly interacts with specialization.

@TomasMikula
Copy link
Contributor Author

Indeed, thank you. I will add a check that differing type arguments must not be specialized.

@TomasMikula TomasMikula changed the title Optimize tailcalls even when called with different type arguments. [WIP] Optimize tailcalls even when called with different type arguments. Sep 9, 2017
@smarter
Copy link
Member

smarter commented Sep 9, 2017

@DarkDimius : Do you know why this restriction exist? Dotty seems to have a similar one (test/files/pos/t9647.scala from this PR does not compile in Dotty because of https://github.com/lampepfl/dotty/blob/851c3c2212b7457dedc9ad1e59ff688b17458db5/compiler/src/dotty/tools/dotc/transform/TailRec.scala#L275)

@TomasMikula TomasMikula force-pushed the polymorphic-tailcalls branch 2 times, most recently from 4194290 to cc82947 Compare September 9, 2017 15:25
@TomasMikula
Copy link
Contributor Author

I added a check that @specialized type parameters are unchanged in the recursive call, and both negative and positive tests involving specialized type parameters.

@hrhino
Copy link
Member

hrhino commented Sep 9, 2017

Why do we only do this in case the method is @tailrec-annotated? The documentation claims that it's

A method annotation which verifies that the method will be compiled with tail call optimization.

and that

If it is present, the compiler will issue an error if the method cannot be optimized into a loop.

Moreover, there are plenty of places on the Internet which claim that tail-call optimization is automatic.

I can't think of a reason why I'd want to write a tail-recursive method, and have it not be optimized because I forgot the @tailrec. For extra fun, when I go to put the annotation on to check, I'll get it optimized after all!

@TomasMikula
Copy link
Contributor Author

@hrhino I guess I just wanted to keep the change conservative.

@TomasMikula
Copy link
Contributor Author

When I enable it wholesale, this test fails. The method invocation it is looking for is optimized away. How should I update that test so that it still checks for scala/bug#8062?

@johnynek
Copy link
Contributor

johnynek commented Sep 9, 2017

Thanks for doing this! I have wanted to take a stab at this. I hope we can make it enabled by default.

@TomasMikula
Copy link
Contributor Author

I checked with Scala 2.10.3 that scala/bug#8062 is reproducible even with the recursive call in non-tail position. I updated the test accordingly so that it doesn't interact with this PR.

(Aside: where does scalac-hash come from?)

@retronym
Copy link
Member

retronym commented Sep 11, 2017

The check might have been there because it badly interacts with specialization.

The restriction to only elim tail calls when type args are forwarded came in a4e5d4a, one day after the genesis of the tailcalls phase. This predated the specialization phase by four years.

@dragos do you recall if the restriction was due to astounding prescience about future interactions with specialization? Or maybe just caution?

I updated the test accordingly so that it doesn't interact with this PR.

Thanks for checking before modifying the test. Adding -g:notailcalls to the compiler options in that test case would also have worked.

Aside: where does scalac-hash come from?

https://github.com/retronym/libscala

The scripts are somewhat out of date because of changes to the format and repository of our per-commit builds. I've got new versions in https://gist.github.com/retronym/6b5cea4d94d51fbcf5d7a0587404492e

@TomasMikula TomasMikula changed the title [WIP] Optimize tailcalls even when called with different type arguments. Optimize tailcalls even when called with different type arguments. Sep 11, 2017
@TomasMikula
Copy link
Contributor Author

Thanks, @retronym. I saw it used in issue descriptions and didn't know where it came from.

@adriaanm
Copy link
Contributor

adriaanm commented Sep 18, 2017

@dragos do you recall if the restriction was due to astounding prescience about future interactions with specialization? Or maybe just caution?

Not him, but my guess would be that at some point we also compiled to a platform that didn't erase types :-) (MSIL)

@Atry
Copy link
Contributor

Atry commented Sep 19, 2017

Looks like related to scala/bug#10493

@adriaanm
Copy link
Contributor

Maybe we should push this phase back until after lambdalift? That would fix both bugs (different polymorphic instantiations of both the method and the receiver), since we're looking at erased (and specialized) types.

@adriaanm adriaanm added the prio:hi high priority (used only by core team, only near release time) label Sep 26, 2017
@TomasMikula
Copy link
Contributor Author

TomasMikula commented Sep 28, 2017

Just changing tailcalls to run right after lambdalift breaks:

  • 9 tests in pos,
  • 16 tests in neg,
  • 31 test in run,
  • 5 tests in jvm,
  • 15 tests in specialized.

@adriaanm
Copy link
Contributor

adriaanm commented Sep 28, 2017

That's pretty good :-) Thanks for trying it out!

@adriaanm
Copy link
Contributor

adriaanm commented Sep 28, 2017

We can start with this PR and take that investigation as a separate one for 2.13.0-M4

@adriaanm adriaanm merged commit 3de8033 into scala:2.12.x Sep 28, 2017
@adriaanm
Copy link
Contributor

Good stuff @TomasMikula!

@TomasMikula
Copy link
Contributor Author

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio:hi high priority (used only by core team, only near release time)
Projects
None yet
9 participants