From ae336b676d0cc23f7ea4da0c66647ccf13928318 Mon Sep 17 00:00:00 2001 From: fhackett Date: Wed, 19 Feb 2020 19:49:35 -0500 Subject: [PATCH] Fix #8306: Ensure the inliner can elide effectively pure case class applications in various situations This is a collection of related changes, all to ensure that the inliner is capable of properly eliding case class constructions in various situations. Specific intended changes: - allow NewInstance.unapply to look past type ascriptions - change all purity checks to consider, for the inliner only, case class applications (when the apply method is synthetic and the case class has no constructor body) pure, or "elideable", despite the fact that the operation is almost always idempotent in the general case. - make reduceInlineMatch's reduceSubPatterns procedure set the defTree for the accessor projections it generates, so nested Unapply patterns can look in defTree to find and possible inline the appropriate subtree of the scrutinee expression - make the typedSelect override able to reduce projections without a corresponding inline match, so that if inlining produces SomeCaseClass(x=3).x the case class construction has a change to be elided (only in an inline context) This commit also includes a series of test cases which use compiletime.show to pretty-print the resulting inlined AST of a series of inline expansions. To avoid cases where a malformed AST pretty-prints but causes bad codegen, the computed run-time value is also printed. One last thing I tested manually was that the bytecode matched the pretty-printing. Mid-work on this commit I found that the pretty printer was hiding several cases where redundant code was left in but not showing up when pretty-printed. This is partially tested by the case where full inlining is not possible, which would also include a redundant binding for the scrutinee if that issue were still present. --- .../src/dotty/tools/dotc/typer/Inliner.scala | 97 +++++++++++++++++-- tests/run/i8306.check | 25 +++++ tests/run/i8306.scala | 93 ++++++++++++++++++ 3 files changed, 205 insertions(+), 10 deletions(-) create mode 100644 tests/run/i8306.check create mode 100644 tests/run/i8306.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Inliner.scala b/compiler/src/dotty/tools/dotc/typer/Inliner.scala index 5c87866c4c3d..467a70348ccf 100644 --- a/compiler/src/dotty/tools/dotc/typer/Inliner.scala +++ b/compiler/src/dotty/tools/dotc/typer/Inliner.scala @@ -424,7 +424,7 @@ class Inliner(call: tpd.Tree, rhsToInline: tpd.Tree)(using Context) { computeParamBindings(tp.resultType, Nil, argss) case tp: MethodType => if argss.isEmpty then - ctx.error(i"mising arguments for inline method $inlinedMethod", call.sourcePos) + ctx.error(i"missing arguments for inline method $inlinedMethod", call.sourcePos) false else tp.paramNames.lazyZip(tp.paramInfos).lazyZip(argss.head).foreach { (name, paramtp, arg) => @@ -477,6 +477,73 @@ class Inliner(call: tpd.Tree, rhsToInline: tpd.Tree)(using Context) { tpe.cls.isContainedIn(inlinedMethod) || tpe.cls.is(Package) + /** Very similar to TreeInfo.isPureExpr, but with the following inliner-only exceptions: + * - synthetic case class apply methods, when the case class constructor is empty, are + * elideable but not pure. Elsewhere, accessing the apply method might cause the initialization + * of a containing object so they are merely idempotent. + */ + object isElideableExpr { + def isStatElideable(tree: Tree)(implicit ctx: Context): Boolean = unsplice(tree) match { + case EmptyTree + | TypeDef(_, _) + | Import(_, _) + | DefDef(_, _, _, _, _) => + true + case vdef @ ValDef(_, _, _) => + if (vdef.symbol.flags is Mutable) false else apply(vdef.rhs) + case _ => + false + } + + def apply(tree: Tree): Boolean = unsplice(tree) match { + case EmptyTree + | This(_) + | Super(_, _) + | Literal(_) => + true + case Ident(_) => + isPureRef(tree) + case Select(qual, _) => + if (tree.symbol.is(Erased)) true + else isPureRef(tree) && apply(qual) + case New(_) | Closure(_, _, _) => + true + case TypeApply(fn, _) => + if (fn.symbol.is(Erased) || fn.symbol == defn.InternalQuoted_typeQuote) true else apply(fn) + case Apply(fn, args) => + def isKnownPureOp(sym: Symbol) = + sym.owner.isPrimitiveValueClass + || sym.owner == defn.StringClass + || defn.pureMethods.contains(sym) + val isCaseClassApply = { + val cls = tree.tpe.classSymbol + val meth = fn.symbol + meth.name == nme.apply && + meth.flags.is(Synthetic) && + meth.owner.linkedClass.is(Case) && + cls.isNoInitsClass + } + if (tree.tpe.isInstanceOf[ConstantType] && isKnownPureOp(tree.symbol) // A constant expression with pure arguments is pure. + || (fn.symbol.isStableMember && !fn.symbol.is(Lazy)) + || fn.symbol.isPrimaryConstructor && fn.symbol.owner.isNoInitsClass) // TODO: include in isStable? + apply(fn) && args.forall(apply) + else if (isCaseClassApply) + args.forall(apply) + else if (fn.symbol.is(Erased)) true + else false + case Typed(expr, _) => + apply(expr) + case Block(stats, expr) => + apply(expr) && stats.forall(isStatElideable) + case Inlined(_, bindings, expr) => + apply(expr) && bindings.forall(isStatElideable) + case NamedArg(_, expr) => + apply(expr) + case _ => + false + } + } + /** Populate `thisProxy` and `paramProxy` as follows: * * 1a. If given type refers to a static this, thisProxy binds it to corresponding global reference, @@ -739,13 +806,16 @@ class Inliner(call: tpd.Tree, rhsToInline: tpd.Tree)(using Context) { Some(meth.owner.linkedClass, args, Nil, false) else None } + case Typed(inner, _) => + // drop the ascribed tpt. We only need it if we can't find a NewInstance + unapply(inner) case Ident(_) => val binding = tree.symbol.defTree for ((cls, reduced, prefix, precomputed) <- unapply(binding)) yield (cls, reduced, prefix, precomputed || binding.isInstanceOf[ValDef]) case Inlined(_, bindings, expansion) => unapplyLet(bindings, expansion) - case Block(stats, expr) if isPureExpr(tree) => + case Block(stats, expr) if isElideableExpr(tree) => unapplyLet(stats, expr) case _ => None @@ -778,13 +848,13 @@ class Inliner(call: tpd.Tree, rhsToInline: tpd.Tree)(using Context) { .reporting(i"projecting $tree -> $result", inlining) val arg = args(idx) if (precomputed) - if (isPureExpr(arg)) finish(arg) + if (isElideableExpr(arg)) finish(arg) else tree // nothing we can do here, projection would duplicate side effect else { // newInstance is evaluated in place, need to reflect side effects of // arguments in the order they were written originally def collectImpure(from: Int, end: Int) = - (from until end).filterNot(i => isPureExpr(args(i))).toList.map(args) + (from until end).filterNot(i => isElideableExpr(args(i))).toList.map(args) val leading = collectImpure(0, idx) val trailing = collectImpure(idx + 1, args.length) val argInPlace = @@ -1041,7 +1111,9 @@ class Inliner(call: tpd.Tree, rhsToInline: tpd.Tree)(using Context) { case (Nil, Nil) => true case (pat :: pats1, selector :: selectors1) => val elem = newSym(InlineBinderName.fresh(), Synthetic, selector.tpe.widenTermRefExpr).asTerm - caseBindingMap += ((NoSymbol, ValDef(elem, constToLiteral(selector)).withSpan(elem.span))) + val rhs = constToLiteral(selector) + elem.defTree = rhs + caseBindingMap += ((NoSymbol, ValDef(elem, rhs).withSpan(elem.span))) reducePattern(caseBindingMap, elem.termRef, pat) && reduceSubPatterns(pats1, selectors1) case _ => false @@ -1143,9 +1215,14 @@ class Inliner(call: tpd.Tree, rhsToInline: tpd.Tree)(using Context) { override def typedSelect(tree: untpd.Select, pt: Type)(using Context): Tree = { assert(tree.hasType, tree) val qual1 = typed(tree.qualifier, selectionProto(tree.name, pt, this)) - val res = constToLiteral(untpd.cpy.Select(tree)(qual1, tree.name).withType(tree.typeOpt)) - ensureAccessible(res.tpe, tree.qualifier.isInstanceOf[untpd.Super], tree.sourcePos) - checkStaging(res) + val resNoReduce = untpd.cpy.Select(tree)(qual1, tree.name).withType(tree.typeOpt) + val resMaybeReduced = constToLiteral(reducer.reduceProjection(resNoReduce)) + if (resNoReduce ne resMaybeReduced) + typed(resMaybeReduced, pt) // redo typecheck if reduction changed something + else + val res = resMaybeReduced + ensureAccessible(res.tpe, tree.qualifier.isInstanceOf[untpd.Super], tree.sourcePos) + checkStaging(res) } private def checkStaging(tree: Tree): tree.type = @@ -1264,8 +1341,8 @@ class Inliner(call: tpd.Tree, rhsToInline: tpd.Tree)(using Context) { val bindingOfSym = newMutableSymbolMap[MemberDef] def isInlineable(binding: MemberDef) = binding match { - case ddef @ DefDef(_, Nil, Nil, _, _) => isPureExpr(ddef.rhs) - case vdef @ ValDef(_, _, _) => isPureExpr(vdef.rhs) + case ddef @ DefDef(_, Nil, Nil, _, _) => isElideableExpr(ddef.rhs) + case vdef @ ValDef(_, _, _) => isElideableExpr(vdef.rhs) case _ => false } for (binding <- bindings if isInlineable(binding)) { diff --git a/tests/run/i8306.check b/tests/run/i8306.check new file mode 100644 index 000000000000..c9437d160771 --- /dev/null +++ b/tests/run/i8306.check @@ -0,0 +1,25 @@ +compile-time: 3 +run-time: 3 +compile-time: 3 +run-time: 3 +compile-time: 3 +run-time: 3 +compile-time: 3 +run-time: 3 +compile-time: { + val $elem9: A = Test.a + val $elem10: Int = $elem9.i + val i: Int = $elem10 + i:Int +} +run-time: 3 +compile-time: 3 +run-time: 3 +compile-time: 3 +run-time: 3 +compile-time: 3 +run-time: 3 +compile-time: 3 +run-time: 3 +compile-time: 3 +run-time: 3 diff --git a/tests/run/i8306.scala b/tests/run/i8306.scala new file mode 100644 index 000000000000..678fc390defd --- /dev/null +++ b/tests/run/i8306.scala @@ -0,0 +1,93 @@ +import scala.compiletime._ + +case class A(i: Int) +case class B(a: A) +case class C[T](t: T) + +trait Test8 { + inline def test8: Int = + inline A(3) match { + case A(i) => i + } +} + +object Test extends Test8 { + + inline def test1: Int = + inline A(3) match { + case A(i) => i + } + + inline def test2: Int = + inline (A(3) : A) match { + case A(i) => i + } + + inline def test3: Int = + inline B(A(3)) match { + case B(A(i)) => i + } + + inline def test4: Int = + A(3).i + + val a = A(3) + inline def test5: Int = + inline new B(a) match { + case B(A(i)) => i + } + + inline def test6: Int = + inline B(A(3)).a match { + case A(i) => i + } + + inline def test7: Int = + inline new A(3) match { + case A(i) => i + } + + inline def test9: Int = + B(A(3)).a.i + + inline def test10: Int = + inline C(3) match { + case C(t) => t + } + + def main(argv: Array[String]): Unit = { + println(code"compile-time: ${test1}") + println(s"run-time: ${test1}") + + println(code"compile-time: ${test2}") + println(s"run-time: ${test2}") + + println(code"compile-time: ${test3}") + println(s"run-time: ${test3}") + + println(code"compile-time: ${test4}") + println(s"run-time: ${test4}") + + // this is the only test that should not be possible to fully inline, + // because it references a non-inline value + println(code"compile-time: ${test5}") + println(s"run-time: ${test5}") + + println(code"compile-time: ${test6}") + println(s"run-time: ${test6}") + + println(code"compile-time: ${test7}") + println(s"run-time: ${test7}") + + println(code"compile-time: ${test8}") + println(s"run-time: ${test8}") + + println(code"compile-time: ${test9}") + println(s"run-time: ${test9}") + + // with type parameter + println(code"compile-time: ${test10}") + println(s"run-time: ${test10}") + } +} +