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

Fix #15494: Handle non-specialized functions in EtaReduce. #15498

Merged
merged 1 commit into from
Jun 22, 2022

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented Jun 22, 2022

In Scala.js, function specialization is disabled. This means that non-specialized functions can reach EtaReduce. Previously, it did not handle the shape of right-hand-side for non-specialized functions returning primitives. We fix the issue by handling those, like we already handled .asInstanceOfs.

How I fixed it

First, I took the reproduction from the issue and put it in sandbox/scalajs/src/hello.scala. I verified that I could reproduce with

> sjsSandbox/run

Next, I looked at the Scala.js IR produced for this code:

> sjsSandbox/scalajsp Thunk$

showed nothing wrong, but

> sjsSandbox/scalajsp hello$package$

showed, sure enough, an extra wrapping when calling asFunction0 for f2 and f3, which is not present for f1:

  def Test;V() {
    val i: scala.runtime.IntRef = scala.runtime.IntRef::create;I;Lscala.runtime.IntRef(0);
    val f1: scala.Function0 = <redacted>;
    if ((!(mod:hello.Thunk$.asFunction0;Lscala.Function0;Lscala.Function0(f1) === f1))) {
      mod:scala.runtime.Scala3RunTime$.assertFailed;E()
    };
    val f2: scala.Function0 = <redacted>;
    if ((!(mod:hello.Thunk$.asFunction0;Lscala.Function0;Lscala.Function0(new scala.scalajs.runtime.AnonFunction0().<init>;Lscala.scalajs.js.Function0;V((arrow-lambda<this$3{this}: hello.hello$package$ = this, f2$2{f2}: scala.Function0 = f2>(): any = {
      this$3.hello.hello$package$::private::Test$$anonfun$1;Lscala.Function0;I(f2$2)
    }))) === f2))) {
      mod:scala.runtime.Scala3RunTime$.assertFailed;E()
    };
    val f3: scala.Function0 = <redacted>;
    if ((!(mod:hello.Thunk$.asFunction0;Lscala.Function0;Lscala.Function0(new scala.scalajs.runtime.AnonFunction0().<init>;Lscala.scalajs.js.Function0;V((arrow-lambda<this$5{this}: hello.hello$package$ = this, f3$2{f3}: scala.Function0 = f3>(): any = {
      this$5.hello.hello$package$::private::Test$$anonfun$adapted$1;Lscala.Function0;Ljava.lang.Object(f3$2)
    }))) === f3))) {
      mod:scala.runtime.Scala3RunTime$.assertFailed;E()
    }
  }

Now I needed to diagnose what phase within the compiler caused the issue. I do that with

> set sjsSandbox / scalacOptions += "-Xprint:flatten"
> sjsSandbox/compile

flatten is always a good first candidate when working on Scala.js issues, as it shows the dotc trees right before the back-end.

flatten shows that the wrappings are already there, so the issue must come earlier. There's not supposed to be major differences in transformations in earlier phases for Scala.js versus Scala/JVM, especially when no Scala.js-specific code is involved.

So I started investigating the difference in -Xprint between Scala/JVM and Scala.js for the same snippet.

I reached the conclusion that the group of etaReduce received as input something worse on Scala.js than Scala/JVM. And for good reason: the input on Scala/JVM uses specialized function types, which don't exist on Scala.js.

I then tried reproducing on Scala/JVM by manually deactivating the SpecializeFunctions phase. Sure enough, the issue was triggered.

From there, it was a matter of looking at the shape of trees received by EtaReduce when SpecializeFunctions is disabled, and adding cases in EtaReduce to handle those shapes.

In Scala.js, function specialization is disabled. This means that
non-specialized functions can reach `EtaReduce`. Previously, it
did not handle the shape of right-hand-side for non-specialized
functions returning primitives. We fix the issue by handling those,
like we already handled `.asInstanceOf`s.
@sjrd sjrd requested a review from odersky June 22, 2022 09:38
@Kordyjan Kordyjan linked an issue Jun 22, 2022 that may be closed by this pull request
@Kordyjan
Copy link
Contributor

As it is a regression from 3.1.2, I think we should backport it to 3.2.0.

@Kordyjan Kordyjan added this to the 3.2.0 backports milestone Jun 22, 2022
@Kordyjan Kordyjan added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Jun 22, 2022
@sjrd
Copy link
Member Author

sjrd commented Jun 22, 2022

What's the procedure for that? Do I retarget this PR to release-3.2.0? Or do we first review and merge in main before cherry-picking to release-3.2.0?

@odersky
Copy link
Contributor

odersky commented Jun 22, 2022

What's the procedure for that? Do I retarget this PR to release-3.2.0? Or do we first review and merge in main before cherry-picking to release-3.2.0?

We merge to main and then backport.

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.

Nice explanation of the fix!

@odersky odersky enabled auto-merge June 22, 2022 10:17
@odersky odersky merged commit 4e5e055 into scala:main Jun 22, 2022
@odersky odersky deleted the eta-reduce-non-specialized-funs branch June 22, 2022 10:57
@Kordyjan Kordyjan added backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Jun 22, 2022
sjrd added a commit that referenced this pull request Jun 22, 2022
Backport #15498: Fix #15494: Handle non-specialized functions in EtaReduce.
@Kordyjan Kordyjan added backport:done This PR was successfully backported. and removed backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" labels Jul 7, 2022
@Kordyjan Kordyjan modified the milestones: 3.2.0 backports, 3.2.1 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:done This PR was successfully backported. how-i-fixed-it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

i14623.scala fails on Scala.js as of 3.1.3
4 participants