Add pattern matcher optimizations #2829

odersky · 2017-07-03T17:09:59Z

We add more extensive optimizations to pattern matching.

In particular: Eliminate redundant tests, and merge variables that have the same right-hand side.

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 ("Added" instead of "Add")

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! ☀️

nicolasstucki · 2017-07-03T18:51:53Z

compiler/src/dotty/tools/dotc/Compiler.scala

@@ -70,7 +70,6 @@ class Compiler {
           new CrossCastAnd,        // Normalize selections involving intersection types.
           new Splitter),           // Expand selections involving union types into conditionals
      List(new VCInlineMethods,     // Inlines calls to value class methods
-           new IsInstanceOfEvaluator, // Issues warnings when unreachable statements are present in match/if expressions


Why is IsInstanceOfEvaluator removed? I see that there are some modifications to it.

@nicolasstucki See #2820

smarter · 2017-07-05T06:48:33Z

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

@@ -530,7 +529,7 @@ object PatternMatcher {
     */
    private def inlineLabelled(plan: Plan) = {
      val refCount = referenceCount(plan)
-      def toDrop(sym: Symbol) = labelled.contains(sym) && refCount(sym) <= 1
+      def toDrop(sym: Symbol) = labelled.contains(sym) && refCount(symb) <= 1


This commit introduces a typo

odersky · 2017-07-06T20:07:48Z

This is ready for review now. With the latest optimizations that do variable propagation across label boundaries the generated code is pretty good now - in fact it looks often better than what one would generate by hand.

odersky · 2017-07-07T08:13:51Z

For instance, here's a translation of the match expression

xs match {
  case 0 :: Nil => println("1")
  case _ :: Nil => println("2")
  case 0 :: _   => println("3")
  case x :: y   => println("4")
}

Translation:

  case val x17: scala.collection.immutable.List[Int](Test.xs) = Test.xs
  def case16(case x30: scala.collection.immutable.::[Int]): Unit = 
    {
      case val x25: Int = x30.head
      case val x26: scala.collection.immutable.List[Int] = x30.tl$1
      if Nil.==(x26) then 
        {
          println("2")
        }
       else 
        if 0.==(x25) then 
          {
            println("3")
          }
         else 
          {
            case val x: Int = x25
            case val y: scala.collection.immutable.List[Int] = x26
            println("4")
          }
    }
  if x17.$isInstanceOf$[scala.collection.immutable.::[Int]] then 
    {
      case val x27: scala.collection.immutable.::[Int] = 
        x17.asInstanceOf[scala.collection.immutable.::[Int]]
      if 0.==(x27.head) then 
        if Nil.==(x27.tl$1) then 
          {
            println("1")
          }
         else case16(x27)
       else case16(x27)
    }
   else throw new MatchError(x17)
}

DarkDimius · 2017-07-07T08:56:30Z

@odersky, looks really good. Here are several more steps that Simplifier in https://github.com/dotty-linker/dotty/tree/opto is able to additionally do:

      if 0.==(x27.head) then 
        if Nil.==(x27.tl$1) then 
          {
            println("1")
          }
         else case16(x27)
       else case16(x27)

It sees that two else branches are the same and reorders statements so that duplicated else branches are joined:

      if (0.==(x27.head)) && (Nil.==(x27.tl$1)) then 
          {
            println("1")
          }
         else case16(x27) //1

This triggers one more rounds of optimizations after inlining case16 that is now called once.

It additionally gets rid of:

x27, as an alias to immutable x17
x30.head && x27.head are aliased, the second one is removed and the first one is reused. x30.head is always computed on paths that computed x27.head and both are immutable. This gets rid of x25.

There is also an optimization in this code that both simplifier and your optimization have missed. They both compare first element with 0 twice on the path. Simplifier wasn't able to remove it due to rewriting //1 that simplified outer test but made analyzing inner tests more complex. My intention here was to use idempotent expression elimination to remove this duplicated computation but the implementation is still not good enough.

odersky · 2017-07-09T16:52:07Z

@DarkDimius God to see that local opts improves further on this. My guess is it's because local opts do many rounds, whereas pm does just one pass.

DarkDimius · 2017-07-09T16:53:26Z

@odersky, I agree with your guess. Is this branch ready for review?

IsInstanceOfEvaluator as prone to mispredict whether the test was generated by a pattern match or not. We fix this by having a special type test symbol that represents tests generated by a pattern match.

We had: scala> null.isInstanceOf[String] -- Warning: <console>:5:17 ----------------------------------------------------- 5 |null.isInstanceOf[String] |^^^^^^^^^^^^^^^^^^^^^^^^^ |this will always yield true if the scrutinee is non-null, since `Null` is a subclass of `String` (will be optimized away) val res0: Boolean = true scala> val x: AnyRef = null val x: AnyRef = null scala> x.isInstanceOf[String] val res1: Boolean = false Fixed by using `isNullable`.

No need to use inheritance here.

Surprisingly, this caused the IDE to crash with a MatchError. (previous pattern matcher, not new one). I could not work out why. But in any case it was the wrong pattern before.

This now special cases `x.isInstance[y.type]` to `x eq y` and similarly for constant types.

Warn if a match is @switch annotated but we could not transform code to a switch.

The new pattern matcher generates switches only for 3 or more cases. Update bytecode tests accordingly.

- better structure - better names - more extensive documentation

For now we keep the previous one around as PatternMatcherOld.scala

It turns out that when we restrict DropGoodCasts to isInstanceOf tests (as opposed to pattern-matcher generated type tests), we do not miss any error messages. Some warnings are still be omitted under optimize, not sure whether we want to fix that.

It seems there are more issues with local optimizations. I did not track down details but in one case positions were wrong after a rewrite. That was tryPatternMatchError.scala. We should track these problems down in a different PR.

Split constructs for var and label binding into two separate abstractions. Also, more systematic organization of optimizations.

Most optimizations are similar in the way they traverse a plan. This commit factors out the commonalities in a base class `PlanTransform`.

Employing label parameters, we can eliminate redundant expressions systematically. This produces now optimal code for reducable.scala.

The new algorithm systematically avoids references to pattern-generated variables. This leads to nicer types and fewer casts in practice. It also avoids the problem that a type refers to a variable that has been eliminated.

We now demand that the number of different constants in the emitted match is the same as the number of different constants in the original match.

The inline labels optimization is no longer needed. It was also incorrect. To verify: Compile classpath/AggregateClassPath with optimization to witness a crash in the backend: Key not found: case3 The optimization incorrectly eliminates the label even though it is called twice by the generated code. maybe it does not detect case labels called with parameters?

DarkDimius

Minor suggestions, except the one about Case flag.

DarkDimius · 2017-07-10T07:41:23Z

compiler/src/dotty/tools/dotc/ast/TreeInfo.scala

+        f1 === f2 && as1.corresponds(as2)(_ === _)
+      case (TypeApply(f1, ts1), TypeApply(f2, ts2)) =>
+        f1 === f2 && ts1.tpes.corresponds(ts2.tpes)(_ =:= _)
+      case _ =>


A lot of code in Dotty has a habbit of needlessly wrapping code in Block(Nil, expr).
It would be nice to add a case here that handles blocks to make it more robust.

DarkDimius · 2017-07-10T07:43:12Z

compiler/src/dotty/tools/dotc/ast/TreeInfo.scala

+      case _ =>
+        false
+    }
+    def hash(implicit ctx: Context): Int =


DarkDimius · 2017-07-10T07:45:20Z

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

+object PatternMatcher {
+  import ast.tpd._
+
+  final val selfCheck = true // debug option, if on we check that no case gets generated twice


Should we leave it enabled?

No, I think we can drop it. On would typically enable it if something funky happens in the backend with double definitions.

DarkDimius · 2017-07-10T07:47:09Z

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

+    /** Was symbol generated by pattern matcher? */
+    private def isPatmatGenerated(sym: Symbol) =
+      sym.is(Synthetic) &&
+      (sym.is(Label) || sym.name.is(PatMatStdBinderName))


We used to use flag Case for this case. I propose we continue using it.
Using Label is ambiguous, as other phases can also generate it, e.g. tailrec.

DarkDimius · 2017-07-10T07:52:35Z

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

+            onFailure)
+        case UnApply(extractor, implicits, args) =>
+          val mt @ MethodType(_) = extractor.tpe.widen
+          var unapp = extractor.appliedTo(ref(scrutinee).ensureConforms(mt.paramInfos.head))


should there be a TypeTest that ensures that scrutinee conforms to mt.paramInfos.head?

The TypeTest is already generated by Typer.

DarkDimius · 2017-07-10T08:00:11Z

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

-            ((casegen: Casegen) => combineExtractors(altTreeMakers :+ TrivialTreeMaker(casegen.one(Literal(Constant(true)))))(casegen))
-            )
-
-          val find


it should also check if initializer refers to vars:

case class Box[T](var a: T) x match { case x @ Box(t) => // this can be filtered by checking that t is not synthetic x.a = null; t case Box(Box(t)) => // this can't, as the breakage happens through an alias to the outer x.asInstanceOf[Box].a = null; t }

Github bug? This comment used to refer to different line, that implements inlining of vals.

DarkDimius · 2017-07-10T08:01:43Z

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

-            ((casegen: Casegen) => combineExtractors(altTreeMakers :+ TrivialTreeMaker(casegen.one(Literal(Constant(true)))))(casegen))
-            )
-
-          val find


why not do it even if it wasn't annotated similar to how tailrec does it?

Github bug? This comment used to refer to lines deciding to use switch that currently only trigger if user asked for it.

DarkDimius · 2017-07-10T08:11:31Z

In private discussion we agreed to change
#2829 (comment)

to use semantic names instead. I'd also propose to extract this method and make it public so that others can call it, instead of hardcoding the current test. I'll need to modify quite a lot of code to use this test instead of checking for case flag ;-).

DarkDimius · 2017-07-10T08:11:51Z

Otherwise LGTM

dottybot reviewed Jul 3, 2017

View reviewed changes

nicolasstucki reviewed Jul 3, 2017

View reviewed changes

DarkDimius modified the milestone: 0.2 Tech Preview Jul 4, 2017

smarter reviewed Jul 5, 2017

View reviewed changes

odersky force-pushed the add-optimize-patmat branch from dc8b198 to f1a669b Compare July 6, 2017 15:54

odersky mentioned this pull request Jul 9, 2017

Change pattern matcher #2822

Closed

odersky added 18 commits July 9, 2017 19:06

Polishings

c6076a0

Add new pattern matcher

886b2a1

Fixes to pattern matcher

b2aa27e

Make IsInstanceOfEvaluator more robust

6fd3971

IsInstanceOfEvaluator as prone to mispredict whether the test was generated by a pattern match or not. We fix this by having a special type test symbol that represents tests generated by a pattern match.

Test outer pointer in pattern matches

f55959c

Fixes to PatMat

e325601

Make TypeTestsCasts an Object

0717f18

No need to use inheritance here.

Fix typo in TypeMap

4057fb2

Surprisingly, this caused the IDE to crash with a MatchError. (previous pattern matcher, not new one). I could not work out why. But in any case it was the wrong pattern before.

Refine tpd.isInstance

a43d8c1

This now special cases `x.isInstance[y.type]` to `x eq y` and similarly for constant types.

Add structural equality === for some trees

003301b

Perform switch optimization in pattern matcher

2e7294c

Warn on failed switch optimization

96dd7fd

Warn if a match is @switch annotated but we could not transform code to a switch.

Update test

ff473ba

The new pattern matcher generates switches only for 3 or more cases. Update bytecode tests accordingly.

Refactorings

499eed4

- better structure - better names - more extensive documentation

Replace previous pattern matcher

fb03273

For now we keep the previous one around as PatternMatcherOld.scala

More docs and some cleanups

40910d8

odersky added 13 commits July 9, 2017 19:06

Turn off local optimizations again

8c4323f

It seems there are more issues with local optimizations. I did not track down details but in one case positions were wrong after a rewrite. That was tryPatternMatchError.scala. We should track these problems down in a different PR.

Some small tweaks

90226bd

More extensive optimizations

0e4e2bd

Split vars and labels

aaf9d7d

Split constructs for var and label binding into two separate abstractions. Also, more systematic organization of optimizations.

Update documentation

eec5732

Fix typo

661f392

Refactor optimizations

e61ef5d

Most optimizations are similar in the way they traverse a plan. This commit factors out the commonalities in a base class `PlanTransform`.

More aggressive variable sharing

b4377dc

Employing label parameters, we can eliminate redundant expressions systematically. This produces now optimal code for reducable.scala.

Fix docs

7ce9e80

Improve sanitize algorithm

8eacccf

The new algorithm systematically avoids references to pattern-generated variables. This leads to nicer types and fewer casts in practice. It also avoids the problem that a type refers to a variable that has been eliminated.

Refine condition when emitted code is a switch

53d8fef

We now demand that the number of different constants in the emitted match is the same as the number of different constants in the original match.

Avoid test-pickling failure.

c97d783

odersky force-pushed the add-optimize-patmat branch from cbf806a to e138568 Compare July 9, 2017 17:06

DarkDimius approved these changes Jul 10, 2017

View reviewed changes

Address reviewers comments

79493df

odersky merged commit 8a93f6e into scala:master Jul 10, 2017

allanrenucci deleted the add-optimize-patmat branch December 14, 2017 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pattern matcher optimizations #2829

Add pattern matcher optimizations #2829

odersky commented Jul 3, 2017

dottybot left a comment

nicolasstucki Jul 3, 2017

smarter Jul 3, 2017

smarter Jul 5, 2017

odersky commented Jul 6, 2017

odersky commented Jul 7, 2017

DarkDimius commented Jul 7, 2017

odersky commented Jul 9, 2017

DarkDimius commented Jul 9, 2017

DarkDimius left a comment

DarkDimius Jul 10, 2017

DarkDimius Jul 10, 2017

DarkDimius Jul 10, 2017

odersky Jul 10, 2017

DarkDimius Jul 10, 2017

DarkDimius Jul 10, 2017

odersky Jul 10, 2017

DarkDimius Jul 10, 2017

DarkDimius Jul 10, 2017

DarkDimius Jul 10, 2017

DarkDimius Jul 10, 2017

DarkDimius commented Jul 10, 2017

DarkDimius commented Jul 10, 2017

Add pattern matcher optimizations #2829

Add pattern matcher optimizations #2829

Conversation

odersky commented Jul 3, 2017

dottybot left a comment

Choose a reason for hiding this comment

Commit Messages

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky commented Jul 6, 2017

odersky commented Jul 7, 2017

DarkDimius commented Jul 7, 2017

odersky commented Jul 9, 2017

DarkDimius commented Jul 9, 2017

DarkDimius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarkDimius commented Jul 10, 2017

DarkDimius commented Jul 10, 2017