Introduce Labeled blocks, and use them in PatternMatcher #4982

sjrd · 2018-08-21T16:07:52Z

A Labeled block is an expression tree of the form

label[T]: {
  expr
}

where expr must conform to the type T. In addition, within expr (but nowhere else), return from the label is allowed:

return[label] e

where e must conform to the type T as well.

If execution of expr completes normally (rather than throwing an exception or returning, etc.), then the result of evaluating the Labeled block is the result of expr. If a return[label] e is reached, the execution of expr is interrupted, and the result of evaluating the Labeled block is the result of evaluating the argument e.

Implementation-wise, a Labeled block is represented as a Tree with the shape:

Labeled(Bind(labelName), expr)

where the Bind nodes holds the definition of the label symbol. That symbol is a term symbol with the flag Label (but not Method, unlike symbols for label-defs) and whose info is the result type T of the labeled block.

We use those new Labeled blocks in PatternMatcher, instead of label-defs. This is the first step towards completely removing label-defs from the compiler.

This commit structurally fixes a few issues:

It fixes PatMat:(add support guards in switches) implement common subexpression elimination #1313 through the mergeTests optimization.
It fixes Erasure tries to box call to label def #4563 because Labeled blocks are erasure-friendly.
It does a big step towards fixing the upstream test t10387: the compiler can get to the back-end on that test, but it produces too much bytecode for a single JVM method. We do add a sister test t10387b which works because optimizations can kick in.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Commit Messages

We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).

Please stick to these guidelines for commit messages:

Separate subject from body with a blank line

When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:

Limit the subject line to 72 characters

Capitalize the subject line

Do not end the subject line with a period

Use the imperative mood in the subject line ("Add" instead of "Added")

Wrap the body at 80 characters

Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

sjrd · 2018-08-21T16:10:16Z

Can anyone tell me how I can get the CI to take into account the version of scala-backend I have pushed at https://github.com/dotty-staging/scala/tree/dotty-labeled?

smarter · 2018-08-21T16:15:02Z

Push it to a branch on http://github.com/lampepfl/scala or replace lampepfl by dotty-staging in .gitmodules under the scala-backend section.

smarter · 2018-08-21T16:16:22Z

compiler/src/dotty/tools/dotc/core/tasty/TastyFormat.scala

-  final val OBJECTDEF = 175
-
-  // In binary: 101100EI
+  final val LABELED = 144


Need to bump the tasty MajorVersion in this file.

I don't think so. I think the last bump to version 10 happened after the latest release.

We should always bump when we break tasty. If we merge and you don't bump, people will start getting confusing error messages in their build when incremental compilation mixes old and new tasty files.

smarter · 2018-08-21T16:19:17Z

compiler/src/dotty/tools/dotc/printing/RefinedPrinter.scala

-        changePrec(GlobalPrec) { keywordStr("return") ~ optText(expr)(" " ~ _) }
+        val sym = from.symbol
+        if (sym.is(Label))
+          changePrec(GlobalPrec) { keywordStr("return@") ~ toText(sym.name) ~ optText(expr)(" " ~ _) }


Very minor bikeshed but I'm not a fan of using @ since it looks too much like an annotation. Maybe write return[foo] instead ?

Sure, whatever ;)

sjrd · 2018-08-21T17:55:01Z

Push it to a branch on http://github.com/lampepfl/scala or replace lampepfl by dotty-staging in .gitmodules under the scala-backend section.

Thanks. That worked :)

sjrd · 2018-08-22T12:32:27Z

Good to review :-D Who feels like it?

smarter · 2018-08-22T12:40:54Z

What's missing to completely remove labeldefs?

smarter · 2018-08-22T12:41:25Z

test performance please

nicolasstucki · 2018-08-22T12:41:32Z

Need to update the TreeChecker to check that label returns are inside their labeled block.

nicolasstucki · 2018-08-22T12:47:27Z

compiler/src/dotty/tools/dotc/core/tasty/TreePickler.scala

          withLength { pickleTree(pat); pickleTree(rhs); pickleTreeUnlessEmpty(guard) }
+        case Labeled(bind, expr) =>
+          writeByte(LABELED)
+          withLength { pickleTree(bind); pickleTree(expr) }


Right now we will never pickle Labeled trees as they are generated in pattern matcher. We should add this code only when we will need it for while and do while

sjrd · 2018-08-22T13:25:50Z

What's missing to completely remove labeldefs?

tailrec-generated labels, and while/do..while loop-generated ones.

smarter · 2018-08-22T13:45:09Z

@liufengyun any idea why "test performance please" above didn't work?

liufengyun · 2018-08-22T14:14:01Z

test performance please

@smarter the polling service ended, just restarted.

dottybot · 2018-08-22T14:17:04Z

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

smarter · 2018-08-22T14:20:51Z

It'd be interesting to check if https://gist.github.com/smarter/1c564e65514d328462e7453162ccfb65 which works in scalac (but was removed from the test suite for being too slow) now works with this PR (it stackoverflows on master).

sjrd · 2018-08-22T14:26:26Z

It "works" as far as the compiler itself is concerned, but it produces too much code for the JVM:

java.lang.RuntimeException: Method integer_of_char's code too large!7s)
        at scala.tools.asm.MethodWriter.getSize(MethodWriter.java:2070)
        at scala.tools.asm.ClassWriter.toByteArray(ClassWriter.java:861)
        at dotty.tools.backend.jvm.GenBCodePipeline$Worker2.getByteArray$1(GenBCode.scala:336)

nicolasstucki · 2018-08-22T14:28:32Z

At least we do not crash before reaching that limit.

smarter · 2018-08-22T14:36:28Z

@sjrd interesting, I suppose that's something that would be fixed with more optimizations in the pattern matcher?

sjrd · 2018-08-22T14:48:23Z

Yes and no. One thing would be to short-circuit unapply calls for case classes. Apparently dotty only does that for Scala2 case classes, but not for its own. (this is part of the "plan building" which I did not touch in this PR)

Other than that, there's not much room for optimization of this particular pattern-match. It is specifically designed to prevent any kind of optimization: the bits are always alternating at the top-level, so the optimizer can never collapse two consecutive cases. It has to redo all the tests from the top every time. scalac does not fare better than my PR, here.

nicolasstucki · 2018-08-22T15:25:33Z

Just for curiosity, if you reorder the cases to not alternate does it optimize and compile?

nicolasstucki · 2018-08-22T15:28:29Z

I mean this code https://gist.github.com/nicolasstucki/36741b8cb63199c8ef442fff1dffaf3f

sjrd · 2018-08-22T15:52:52Z

Yes, it does. The tree after patmat is https://gist.github.com/sjrd/803ddf59e4d17a5ffa445e3f3c16dfd4 (around 5 kLoC, versus 17 kLoC for the original, failing one).

smarter · 2018-08-22T15:54:57Z

compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala

-      "elimRedundantTests" -> elimRedundantTests,
-      "inlineLabelled" -> inlineLabelled,
-      "mergeVars" -> mergeVars,
+      "mergeTests" -> mergeTests,


Could the removed optimizations be brought back later? Or are they subsumed by mergeTests ?

They are indeed subsumed by mergeTests, combined with the inherent simpler nature of Labeled versus label-defs (e.g., hoistLabels is totally unnecessary with Labeled).

dottybot · 2018-08-22T17:55:11Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (6ee3c62)

nicolasstucki

Remove changes from pickling for now. Add them back to the PR that will handle while/do while loops.

smarter · 2018-08-23T17:09:55Z

Would be good to verify that this PR fixes #4563

sjrd · 2018-08-24T09:50:02Z

Remove changes from pickling for now.

Done.

Would be good to verify that this PR fixes #4563

It does. I added a test case.

allanrenucci · 2018-08-24T10:04:52Z

tests/pos/i4563.scala

@@ -0,0 +1,12 @@
+case class Foo(str: String)


This test already compiles and runs on master. TBH, I am not sure this minimises the original issue.

Anyway, this was reported as a runtime issue, so this should be a run test

smarter · 2018-08-24T10:27:26Z

tests/pos/i4563.scala

@@ -0,0 +1,12 @@
+case class Foo(str: String)


This is missing the extends AnyVal

sjrd · 2018-08-24T16:10:05Z

Need to update the TreeChecker to check that label returns are inside their labeled block.

Done.

I think I've addressed all the comments that have been raised so far.

smarter · 2018-08-24T20:13:08Z

Assigning to @odersky since he wrote most of the current PatternMatcher

sjrd · 2018-08-25T22:42:05Z

This PR also fixes #1313. Test case to be added when it's not 1am:

object HelloWorld {
  def main(args: Array[String]): Unit = println("hello world")

  def test(x: Any, y: Int): Int = {
    x match {
      case List(1, 2, 3) => 1
      case List(3, 4, 5) => 2
      case 6 if y > 5 => 3
      case 6 => 4
      case _ => 5
    }
  }

  def test2(x: Int, y: Int): Int = {
    (x: @scala.annotation.switch) match {
      case 6 if y > 5 => 1
      case 6 => 2
      case 12 => 3
      case 14 => 4
      case _ => 5
    }
  }
}

No "switch" warning! Tree after the pattern matcher:

package <empty> {
  final lazy module val HelloWorld: HelloWorld$ = new HelloWorld$()
  @scala.annotation.internal.SourceFile("tests\\pos\\HelloWorld.scala")
    final module
   class HelloWorld$() extends Object() {
    def main(args: Array[String]): Unit = println("hello world")
    def test(x: Any, y: Int): Int =
      {
        matchResult1[Int]:
          {
            case val x1: Any(x) = x
            if x1.$isInstanceOf$[List[Any]] then
              {
                case val x8: List[Any] = x1.asInstanceOf[List[Any]]
                case val x9: Some[List[Any]] = List.unapplySeq[Any](x8)
                if x9.isEmpty.unary_! then
                  {
                    case val x10: List[Any] = x9.get
                    if x10.lengthCompare(3).==(0) then
                      {
                        case val x11: Any = x10.apply(0)
                        case val x12: Any = x10.apply(1)
                        case val x13: Any = x10.apply(2)
                        if 1.==(x11).&&(2.==(x12)).&&(3.==(x13)) then
                          return[matchResult1] 1
                         else ()
                        if 3.==(x11).&&(4.==(x12)).&&(5.==(x13)) then
                          return[matchResult1] 2
                         else ()
                      }
                     else ()
                  }
                 else ()
              }
             else ()
            if 6.==(x1) then
              {
                if y.>(5) then return[matchResult1] 3 else ()
                return[matchResult1] 4
              }
             else ()
            return[matchResult1] 5
            return[matchResult1] throw new MatchError(x1)
          }
      }
    def test2(x: Int, y: Int): Int =
      {
        matchResult2[Int]:
          {
            case val x14: Int(x) @switch = x: Int(x) @switch
            x14 match
              {
                case 6 =>
                  if y.>(5) then return[matchResult2] 1 else ()
                  return[matchResult2] 2
                case 12 => return[matchResult2] 3
                case 14 => return[matchResult2] 4
                case _ =>
                  return[matchResult2] 5
                  return[matchResult2] throw new MatchError(x14)
              }
          }
      }
  }
}

This is consistent with those produced by the current pattern matcher.

odersky

I love the simplifications!

odersky · 2018-08-27T14:07:30Z

compiler/src/dotty/tools/dotc/core/NameKinds.scala

-  val PatMatMatchFailName     = new UniqueNameKind("matchFail")
-  val PatMatSelectorName      = new UniqueNameKind("selector")
+  val PatMatAltsName          = new UniqueNameKind("matchAlts")
+  val PatMatResultName        = new UniqueNameKind("matchResult")


nice simplification!

odersky · 2018-08-27T16:42:32Z

Good to merge unless @sjrd has something to add.

sjrd · 2018-08-27T16:44:09Z

Thank you :)

I don't have anything to add. Changes to loops and tailrec are best left as separate PRs. Let's ship it!

sjrd · 2018-08-27T16:46:56Z

Oh, I'm just going to update the submodule commit reference to the new HEAD of sharing-backend (the merge commit).

A Labeled block is an expression tree of the form label[T]: { expr } where `expr` must conform to the type `T`. In addition, within `expr` (but nowhere else), return from the label is allowed: return[label] e where `e` must conform to the type `T` as well. If execution of `expr` completes normally (rather than throwing an exception or returning, etc.), then the result of evaluating the `Labeled` block is the result of `expr`. If a `return[label] e` is reached, the execution of `expr` is interrupted, and the result of evaluating the `Labeled` block is the result of evaluating the argument `e`. Implementation-wise, a `Labeled` block is represented as a `Tree` with the shape: Labeled(Bind(labelName), expr) where the `Bind` nodes holds the definition of the label symbol. That symbol is a term symbol with the flag `Label` (but not `Method`, unlike symbols for label-defs) and whose `info` is the result type `T` of the labeled block. We use those new `Labeled` blocks in `PatternMatcher`, instead of label-defs. This is the first step towards completely removing label-defs from the compiler. This commit structurally fixes a few issues: * It fixes scala#1313 through the `mergeTests` optimization. * It fixes scala#4563 because Labeled blocks are erasure-friendly. * It does a big step towards fixing the upstream test t10387: the compiler can get to the back-end on that test, but it produces too much bytecode for a single JVM method. We do add a sister test t10387b which works because optimizations can kick in.

dottybot reviewed Aug 21, 2018

View reviewed changes

smarter reviewed Aug 21, 2018

View reviewed changes

sjrd force-pushed the labeled branch from cfc08a0 to 0ed6d41 Compare August 21, 2018 17:53

sjrd force-pushed the labeled branch 2 times, most recently from 9c19830 to 432aaaa Compare August 22, 2018 12:30

sjrd changed the title ~~[WIP CI only] Labeled blocks~~ Introduce Labeled blocks, and use them in PatternMatcher. Aug 22, 2018

sjrd mentioned this pull request Aug 22, 2018

Add support for Labeled blocks in dotty. lampepfl/scala#36

Merged

nicolasstucki reviewed Aug 22, 2018

View reviewed changes

nicolasstucki changed the title ~~Introduce Labeled blocks, and use them in PatternMatcher.~~ Introduce Labeled blocks, and use them in PatternMatcher Aug 22, 2018

smarter reviewed Aug 22, 2018

View reviewed changes

nicolasstucki suggested changes Aug 23, 2018

View reviewed changes

allanrenucci reviewed Aug 24, 2018

View reviewed changes

smarter reviewed Aug 24, 2018

View reviewed changes

tests/pos/i4563.scala Outdated

@@ -0,0 +1,12 @@

case class Foo(str: String)

Copy link

Member

smarter Aug 24, 2018

Choose a reason for hiding this comment

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

This is missing the extends AnyVal

sjrd mentioned this pull request Aug 24, 2018

Erasure tries to box call to label def #4563

Closed

smarter assigned odersky Aug 24, 2018

smarter requested a review from odersky August 24, 2018 20:11

sjrd force-pushed the labeled branch from e212731 to 613e725 Compare August 24, 2018 23:08

sjrd force-pushed the labeled branch from 613e725 to c3e9382 Compare August 26, 2018 14:32

Give the Method flag to the label-defs produced by TailRec.

994df27

This is consistent with those produced by the current pattern matcher.

sjrd force-pushed the labeled branch from c3e9382 to 2a74482 Compare August 26, 2018 14:42

odersky approved these changes Aug 27, 2018

View reviewed changes

odersky assigned sjrd and unassigned odersky Aug 27, 2018

sjrd force-pushed the labeled branch from 2a74482 to 4c53008 Compare August 27, 2018 16:48

sjrd merged commit 85793ba into scala:master Aug 27, 2018

sjrd deleted the labeled branch August 27, 2018 17:29

allanrenucci mentioned this pull request Sep 3, 2018

Better support for guards in switches #5070

Open

Introduce Labeled blocks, and use them in PatternMatcher #4982

Introduce Labeled blocks, and use them in PatternMatcher #4982

Uh oh!

Conversation

sjrd commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Commit Messages

Uh oh!

sjrd commented Aug 21, 2018

Uh oh!

smarter commented Aug 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjrd commented Aug 21, 2018

Uh oh!

sjrd commented Aug 22, 2018

Uh oh!

smarter commented Aug 22, 2018

Uh oh!

smarter commented Aug 22, 2018

Uh oh!

nicolasstucki commented Aug 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjrd commented Aug 22, 2018

Uh oh!

smarter commented Aug 22, 2018

Uh oh!

liufengyun commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dottybot commented Aug 22, 2018

Uh oh!

smarter commented Aug 22, 2018

Uh oh!

sjrd commented Aug 22, 2018

Uh oh!

nicolasstucki commented Aug 22, 2018

Uh oh!

smarter commented Aug 22, 2018

Uh oh!

sjrd commented Aug 22, 2018

Uh oh!

nicolasstucki commented Aug 22, 2018

Uh oh!

nicolasstucki commented Aug 22, 2018

Uh oh!

sjrd commented Aug 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjrd Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dottybot commented Aug 22, 2018

Uh oh!

nicolasstucki left a comment

Choose a reason for hiding this comment

Uh oh!

smarter commented Aug 23, 2018

Uh oh!

sjrd commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sjrd commented Aug 21, 2018 •

edited

Loading

liufengyun commented Aug 22, 2018 •

edited

Loading

sjrd Aug 22, 2018 •

edited

Loading