From 6db997682a31f959097e724378d1dcb0ad817f19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Doeraene?= Date: Wed, 22 Jul 2020 16:26:03 +0200 Subject: [PATCH 1/2] For switch-Matches in PatMap, convert scrutinee and alternatives to Int. In JVM bytecode as well in Scala.js IR, switches only work with primitive ints. Therefore, it makes more sense to convert the scrutinee and alternatives of switch-Matches to Ints early. This is also what scalac does. See `RegularSwitchMaker` in `patmat/MatchOptimization.scala`. The JVM back-end tolerates non-ints due its aggressive and blind adaptations everywhere (not because of a deliberate action to support non-Ints). However, the Scala.js back-end does not like receiving non-Ints in `Match`es, because it is much more conservative in where it inserts adaptations (i.e., almost nowhere). --- .../tools/dotc/transform/PatternMatcher.scala | 32 +++++++++++++------ 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala b/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala index d396d8f75247..0e63cb898a09 100644 --- a/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala +++ b/compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala @@ -821,17 +821,31 @@ object PatternMatcher { } } - /** Emit cases of a switch */ - private def emitSwitchCases(cases: List[(List[Tree], Plan)]): List[CaseDef] = (cases: @unchecked) match { - case (alts, ons) :: cases1 => + /** Emit a switch-match */ + private def emitSwitchMatch(scrutinee: Tree, cases: List[(List[Tree], Plan)]): Match = { + /* Make sure to adapt the scrutinee to Int, as well as all the alternatives + * of all cases, so that only Matches on pritimive Ints survive this phase. + */ + + val intScrutinee = + if (scrutinee.tpe.widen.isRef(defn.IntClass)) scrutinee + else scrutinee.select(nme.toInt) + + def intLiteral(lit: Tree): Tree = + val Literal(constant) = lit + if (constant.tag == Constants.IntTag) lit + else cpy.Literal(lit)(Constant(constant.intValue)) + + val caseDefs = cases.map { (alts, ons) => val pat = alts match { - case alt :: Nil => alt + case alt :: Nil => intLiteral(alt) case Nil => Underscore(defn.IntType) // default case - case _ => Alternative(alts) + case _ => Alternative(alts.map(intLiteral)) } - CaseDef(pat, EmptyTree, emit(ons)) :: emitSwitchCases(cases1) - case nil => - Nil + CaseDef(pat, EmptyTree, emit(ons)) + } + + Match(intScrutinee, caseDefs) } /** If selfCheck is `true`, used to check whether a tree gets generated twice */ @@ -892,7 +906,7 @@ object PatternMatcher { def maybeEmitSwitch(scrutinee: Tree): Tree = { val switchCases = collectSwitchCases(scrutinee, plan) if (hasEnoughSwitchCases(switchCases, MinSwitchCases)) // at least 3 cases + default - Match(scrutinee, emitSwitchCases(switchCases)) + emitSwitchMatch(scrutinee, switchCases) else default } From bb5a93b039cea945295f0a8ba3baf8e206fde6ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Doeraene?= Date: Wed, 22 Jul 2020 16:41:15 +0200 Subject: [PATCH 2/2] Fix #9268: Scala.js: Add support for switch-Matches. This implementation has been ported from the scalac backend, but is much simpler because it does not have to deal with hacks around scalac's LabelDefs. --- .../dotty/tools/backend/sjs/JSCodeGen.scala | 127 +++++++++++++++++- project/Build.scala | 1 - 2 files changed, 123 insertions(+), 5 deletions(-) diff --git a/compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala b/compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala index d93160e72f12..6c20c57e277f 100644 --- a/compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala +++ b/compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala @@ -30,7 +30,7 @@ import dotty.tools.dotc.util.Spans.Span import dotty.tools.dotc.report import org.scalajs.ir -import org.scalajs.ir.{ClassKind, Position, Trees => js, Types => jstpe} +import org.scalajs.ir.{ClassKind, Position, Names => jsNames, Trees => js, Types => jstpe} import org.scalajs.ir.Names.{ClassName, MethodName, SimpleMethodName} import org.scalajs.ir.OriginalName import org.scalajs.ir.OriginalName.NoOriginalName @@ -1176,9 +1176,7 @@ class JSCodeGen()(using genCtx: Context) { /** A Match reaching the backend is supposed to be optimized as a switch */ case mtch: Match => - // TODO Correctly handle `Match` nodes - //genMatch(mtch, isStat) - js.Throw(js.Null()) + genMatch(mtch, isStat) case tree: Closure => genClosure(tree) @@ -2228,6 +2226,127 @@ class JSCodeGen()(using genCtx: Context) { js.ArrayValue(arrayTypeRef, genElems) } + /** Gen JS code for a switch-`Match`, which is translated into an IR `js.Match`. */ + def genMatch(tree: Tree, isStat: Boolean): js.Tree = { + implicit val pos = tree.span + val Match(selector, cases) = tree + + def abortMatch(msg: String): Nothing = + throw new FatalError(s"$msg in switch-like pattern match at ${tree.span}: $tree") + + /* Although GenBCode adapts the scrutinee and the cases to `int`, only + * true `int`s can reach the back-end, as asserted by the String-switch + * transformation in `cleanup`. Therefore, we do not adapt, preserving + * the `string`s and `null`s that come out of the pattern matching in + * Scala 2.13.2+. + */ + val genSelector = genExpr(selector) + + // Sanity check: we can handle Ints and Strings (including `null`s), but nothing else + genSelector.tpe match { + case jstpe.IntType | jstpe.ClassType(jsNames.BoxedStringClass) | jstpe.NullType | jstpe.NothingType => + // ok + case _ => + abortMatch(s"Invalid selector type ${genSelector.tpe}") + } + + val resultType = + if (isStat) jstpe.NoType + else toIRType(tree.tpe) + + var clauses: List[(List[js.Tree], js.Tree)] = Nil + var optDefaultClause: Option[js.Tree] = None + + for (caze @ CaseDef(pat, guard, body) <- cases) { + if (guard != EmptyTree) + abortMatch("Found a case guard") + + val genBody = genStatOrExpr(body, isStat) + + pat match { + case lit: Literal => + clauses = (List(genExpr(lit)), genBody) :: clauses + case Ident(nme.WILDCARD) => + optDefaultClause = Some(genBody) + case Alternative(alts) => + val genAlts = alts.map { + case lit: Literal => genExpr(lit) + case _ => abortMatch("Invalid case in alternative") + } + clauses = (genAlts, genBody) :: clauses + case _ => + abortMatch("Invalid case pattern") + } + } + + clauses = clauses.reverse + val defaultClause = optDefaultClause.getOrElse { + throw new AssertionError("No elseClause in pattern match") + } + + /* Builds a `js.Match`, but simplifies it to a `js.If` if there is only + * one case with one alternative, and to a `js.Block` if there is no case + * at all. This happens in practice in the standard library. Having no + * case is a typical product of `match`es that are full of + * `case n if ... =>`, which are used instead of `if` chains for + * convenience and/or readability. + * + * When no optimization applies, and any of the case values is not a + * literal int, we emit a series of `if..else` instead of a `js.Match`. + * This became necessary in 2.13.2 with strings and nulls. + * + * Note that dotc has not adopted String-switch-Matches yet, so these code + * paths are dead code at the moment. However, they already existed in the + * scalac, so were ported, to be immediately available and working when + * dotc starts emitting switch-Matches on Strings. + */ + def isInt(tree: js.Tree): Boolean = tree.tpe == jstpe.IntType + + clauses match { + case Nil => + // Completely remove the Match. Preserve the side-effects of `genSelector`. + js.Block(exprToStat(genSelector), defaultClause) + + case (uniqueAlt :: Nil, caseRhs) :: Nil => + /* Simplify the `match` as an `if`, so that the optimizer has less + * work to do, and we emit less code at the end of the day. + * Use `Int_==` instead of `===` if possible, since it is a common case. + */ + val op = + if (isInt(genSelector) && isInt(uniqueAlt)) js.BinaryOp.Int_== + else js.BinaryOp.=== + js.If(js.BinaryOp(op, genSelector, uniqueAlt), caseRhs, defaultClause)(resultType) + + case _ => + if (isInt(genSelector) && + clauses.forall(_._1.forall(_.isInstanceOf[js.IntLiteral]))) { + // We have int literals only: use a js.Match + val intClauses = clauses.asInstanceOf[List[(List[js.IntLiteral], js.Tree)]] + js.Match(genSelector, intClauses, defaultClause)(resultType) + } else { + // We have other stuff: generate an if..else chain + val (tempSelectorDef, tempSelectorRef) = genSelector match { + case varRef: js.VarRef => + (js.Skip(), varRef) + case _ => + val varDef = js.VarDef(freshLocalIdent(), NoOriginalName, + genSelector.tpe, mutable = false, genSelector) + (varDef, varDef.ref) + } + val ifElseChain = clauses.foldRight(defaultClause) { (caze, elsep) => + val conds = caze._1.map { caseValue => + js.BinaryOp(js.BinaryOp.===, tempSelectorRef, caseValue) + } + val cond = conds.reduceRight[js.Tree] { (left, right) => + js.If(left, js.BooleanLiteral(true), right)(jstpe.BooleanType) + } + js.If(cond, caze._2, elsep)(resultType) + } + js.Block(tempSelectorDef, ifElseChain) + } + } + } + /** Gen JS code for a closure. * * Input: a `Closure` tree of the form diff --git a/project/Build.scala b/project/Build.scala index 28a2d90f3074..30a9392e0bad 100644 --- a/project/Build.scala +++ b/project/Build.scala @@ -1047,7 +1047,6 @@ object Build { -- "CollectionsOnSynchronizedSetTest.scala" -- "CollectionsTest.scala" -- "EventObjectTest.scala" - -- "FormatterTest.scala" -- "HashSetTest.scala" -- "LinkedHashSetTest.scala" -- "SortedSetTest.scala"