From 5256eb4a4f13b248fe595a51e30fb851364c9308 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Doeraene?= Date: Wed, 6 Apr 2022 15:06:20 +0200 Subject: [PATCH 1/2] Add a bytecode test with the status quo of tailrec methods. --- .../tools/backend/jvm/DottyBytecodeTest.scala | 3 +- .../backend/jvm/DottyBytecodeTests.scala | 107 ++++++++++++++++++ 2 files changed, 108 insertions(+), 2 deletions(-) diff --git a/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTest.scala b/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTest.scala index 6c796c280723..ce887ec56ba9 100644 --- a/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTest.scala +++ b/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTest.scala @@ -122,7 +122,7 @@ trait DottyBytecodeTest { def assertSameCode(method: MethodNode, expected: List[Instruction]): Unit = assertSameCode(instructionsFromMethod(method).dropNonOp, expected) def assertSameCode(actual: List[Instruction], expected: List[Instruction]): Unit = { - assert(actual === expected, s"\nExpected: $expected\nActual : $actual") + assert(actual === expected, "\n" + diffInstructions(actual, expected)) } def assertInvoke(m: MethodNode, receiver: String, method: String): Unit = @@ -296,4 +296,3 @@ trait DottyBytecodeTest { object DottyBytecodeTest { extension [T](l: List[T]) def stringLines = l.mkString("\n") } - diff --git a/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala b/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala index 051dd414e28f..0acf19e341bd 100644 --- a/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala +++ b/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala @@ -946,6 +946,113 @@ class TestBCode extends DottyBytecodeTest { } } + @Test def i14773TailRecReuseParamSlots(): Unit = { + val source = + s"""class Foo { + | @scala.annotation.tailrec // explicit @tailrec here + | final def fact(n: Int, acc: Int): Int = + | if n == 0 then acc + | else fact(n - 1, acc * n) + |} + | + |class IntList(head: Int, tail: IntList | Null) { + | // implicit @tailrec + | final def sum(acc: Int): Int = + | val t = tail + | if t == null then acc + head + | else t.sum(acc + head) + |} + """.stripMargin + + checkBCode(source) { dir => + // The mutable local vars for n and acc reuse the slots of the params n and acc + + val fooClass = loadClassNode(dir.lookupName("Foo.class", directory = false).input) + val factMeth = getMethod(fooClass, "fact") + + assertSameCode(factMeth, List( + VarOp(ALOAD, 0), + VarOp(ASTORE, 3), + VarOp(ILOAD, 2), + VarOp(ISTORE, 4), + VarOp(ILOAD, 1), + VarOp(ISTORE, 5), + Label(6), + VarOp(ILOAD, 5), + Op(ICONST_0), + Jump(IF_ICMPNE, Label(13)), + VarOp(ILOAD, 4), + Jump(GOTO, Label(32)), + Label(13), + VarOp(ALOAD, 3), + VarOp(ASTORE, 6), + VarOp(ILOAD, 5), + Op(ICONST_1), + Op(ISUB), + VarOp(ISTORE, 7), + VarOp(ILOAD, 4), + VarOp(ILOAD, 5), + Op(IMUL), + VarOp(ISTORE, 8), + VarOp(ALOAD, 6), + VarOp(ASTORE, 3), + VarOp(ILOAD, 7), + VarOp(ISTORE, 5), + VarOp(ILOAD, 8), + VarOp(ISTORE, 4), + Jump(GOTO, Label(35)), + Label(32), + Op(IRETURN), + Label(35), + Jump(GOTO, Label(6)), + Op(ATHROW), + Op(ATHROW), + )) + + // The mutable local vars for this and acc reuse the slots of `this` and of the param acc + + val intListClass = loadClassNode(dir.lookupName("IntList.class", directory = false).input) + val sumMeth = getMethod(intListClass, "sum") + + assertSameCode(sumMeth, List( + VarOp(ALOAD, 0), + VarOp(ASTORE, 2), + VarOp(ILOAD, 1), + VarOp(ISTORE, 3), + Label(4), + VarOp(ALOAD, 2), + Field(GETFIELD, "IntList", "tail", "LIntList;"), + VarOp(ASTORE, 4), + VarOp(ALOAD, 4), + Jump(IFNONNULL, Label(16)), + VarOp(ILOAD, 3), + VarOp(ALOAD, 2), + Field(GETFIELD, "IntList", "head", "I"), + Op(IADD), + Jump(GOTO, Label(30)), + Label(16), + VarOp(ALOAD, 4), + VarOp(ASTORE, 5), + VarOp(ILOAD, 3), + VarOp(ALOAD, 2), + Field(GETFIELD, "IntList", "head", "I"), + Op(IADD), + VarOp(ISTORE, 6), + VarOp(ALOAD, 5), + VarOp(ASTORE, 2), + VarOp(ILOAD, 6), + VarOp(ISTORE, 3), + Jump(GOTO, Label(33)), + Label(30), + Op(IRETURN), + Label(33), + Jump(GOTO, Label(4)), + Op(ATHROW), + Op(ATHROW), + )) + } + } + @Test def getClazz: Unit = { val source = """ From 730883f4be3a34453e9a38c581a62ae5733225a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Doeraene?= Date: Wed, 6 Apr 2022 17:08:24 +0200 Subject: [PATCH 2/2] Fix #14773: Reuse the param slots for the tailrec local mutable vars. The tailrec phase generates code that looks like class Bar { def foo(x1: Int, x2: Int): Int = { var this$: Bar = this; var taillocal$x1: Int = x1; var taillocal$x2: Int = x2; ... // body where `this`, `x1` and `x2` are replaced by // `this$`, `taillocal$x1` and `taillocal$x2` } } This generates bytecode where the `this` value and the parameters never actually change, and are never used. Instead, the synthetic mutable variables are used instead. As described in the linked issue, this confuses debuggers, which only display the never-changing original `this` value and parameters. In this commit, we intercept this shape of code in the back-end. We reliable identify tailrec-generated `ValDef`s from their semantic names, with an additional safety check that they are `Synthetic | Mutable`. When we find this shape, we do not allocate local slots for the `var`s, and instead reuse the slots for `this` and the parameters. We skip past the `ValDef`s so that code generation does not re-emit useless assignments. --- .../tools/backend/jvm/BCodeSkelBuilder.scala | 47 +++++++++- .../dotty/tools/dotc/transform/TailRec.scala | 2 +- .../backend/jvm/DottyBytecodeTests.scala | 90 +++++++++---------- 3 files changed, 84 insertions(+), 55 deletions(-) diff --git a/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala b/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala index f668a1b68a5f..824a93ed506f 100644 --- a/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala +++ b/compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala @@ -4,6 +4,8 @@ package jvm import scala.language.unsafeNulls +import scala.annotation.tailrec + import scala.collection.{ mutable, immutable } import scala.tools.asm @@ -484,6 +486,14 @@ trait BCodeSkelBuilder extends BCodeHelpers { slots.getOrElse(locSym, makeLocal(locSym)) } + def reuseLocal(sym: Symbol, loc: Local): Unit = + val existing = slots.put(sym, loc) + if (existing.isDefined) + report.error("attempt to create duplicate local var.", ctx.source.atSpan(sym.span)) + + def reuseThisSlot(sym: Symbol): Unit = + reuseLocal(sym, Local(symInfoTK(sym), sym.javaSimpleName, 0, sym.is(Synthetic))) + private def makeLocal(sym: Symbol, tk: BType): Local = { assert(nxtIdx != -1, "not a valid start index") val loc = Local(tk, sym.javaSimpleName, nxtIdx, sym.is(Synthetic)) @@ -753,18 +763,47 @@ trait BCodeSkelBuilder extends BCodeHelpers { .addFlagIf(isNative, asm.Opcodes.ACC_NATIVE) // native methods of objects are generated in mirror classes // TODO needed? for(ann <- m.symbol.annotations) { ann.symbol.initialize } - initJMethod(flags, params.map(_.symbol)) + val paramSyms = params.map(_.symbol) + initJMethod(flags, paramSyms) if (!isAbstractMethod && !isNative) { + // #14773 Reuse locals slots for tailrec-generated mutable vars + val trimmedRhs: Tree = + @tailrec def loop(stats: List[Tree]): List[Tree] = + stats match + case (tree @ ValDef(TailLocalName(_, _), _, _)) :: rest if tree.symbol.isAllOf(Mutable | Synthetic) => + tree.rhs match + case This(_) => + locals.reuseThisSlot(tree.symbol) + loop(rest) + case rhs: Ident if paramSyms.contains(rhs.symbol) => + locals.reuseLocal(tree.symbol, locals(rhs.symbol)) + loop(rest) + case _ => + stats + case _ => + stats + end loop + + rhs match + case Block(stats, expr) => + val trimmedStats = loop(stats) + if trimmedStats eq stats then + rhs + else + Block(trimmedStats, expr) + case _ => + rhs + end trimmedRhs def emitNormalMethodBody(): Unit = { val veryFirstProgramPoint = currProgramPoint() - genLoad(rhs, returnType) + genLoad(trimmedRhs, returnType) - rhs match { + trimmedRhs match { case (_: Return) | Block(_, (_: Return)) => () - case (_: Apply) | Block(_, (_: Apply)) if rhs.symbol eq defn.throwMethod => () + case (_: Apply) | Block(_, (_: Apply)) if trimmedRhs.symbol eq defn.throwMethod => () case tpd.EmptyTree => report.error("Concrete method has no definition: " + dd + ( if (ctx.settings.Ydebug.value) "(found: " + methSymbol.owner.info.decls.toList.mkString(", ") + ")" diff --git a/compiler/src/dotty/tools/dotc/transform/TailRec.scala b/compiler/src/dotty/tools/dotc/transform/TailRec.scala index 9ee3aa5b1a8c..c2555c3d5b95 100644 --- a/compiler/src/dotty/tools/dotc/transform/TailRec.scala +++ b/compiler/src/dotty/tools/dotc/transform/TailRec.scala @@ -253,7 +253,7 @@ class TailRec extends MiniPhase { val tpe = if (enclosingClass.is(Module)) enclosingClass.thisType else enclosingClass.classInfo.selfType - val sym = newSymbol(method, nme.SELF, Synthetic | Mutable, tpe) + val sym = newSymbol(method, TailLocalName.fresh(nme.SELF), Synthetic | Mutable, tpe) varForRewrittenThis = Some(sym) sym } diff --git a/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala b/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala index 0acf19e341bd..9c5f9c167bf9 100644 --- a/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala +++ b/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala @@ -971,41 +971,35 @@ class TestBCode extends DottyBytecodeTest { val factMeth = getMethod(fooClass, "fact") assertSameCode(factMeth, List( + Label(0), + VarOp(ILOAD, 1), + Op(ICONST_0), + Jump(IF_ICMPNE, Label(7)), + VarOp(ILOAD, 2), + Jump(GOTO, Label(26)), + Label(7), VarOp(ALOAD, 0), VarOp(ASTORE, 3), - VarOp(ILOAD, 2), + VarOp(ILOAD, 1), + Op(ICONST_1), + Op(ISUB), VarOp(ISTORE, 4), + VarOp(ILOAD, 2), VarOp(ILOAD, 1), + Op(IMUL), VarOp(ISTORE, 5), - Label(6), - VarOp(ILOAD, 5), - Op(ICONST_0), - Jump(IF_ICMPNE, Label(13)), - VarOp(ILOAD, 4), - Jump(GOTO, Label(32)), - Label(13), VarOp(ALOAD, 3), - VarOp(ASTORE, 6), - VarOp(ILOAD, 5), - Op(ICONST_1), - Op(ISUB), - VarOp(ISTORE, 7), + VarOp(ASTORE, 0), VarOp(ILOAD, 4), + VarOp(ISTORE, 1), VarOp(ILOAD, 5), - Op(IMUL), - VarOp(ISTORE, 8), - VarOp(ALOAD, 6), - VarOp(ASTORE, 3), - VarOp(ILOAD, 7), - VarOp(ISTORE, 5), - VarOp(ILOAD, 8), - VarOp(ISTORE, 4), - Jump(GOTO, Label(35)), - Label(32), + VarOp(ISTORE, 2), + Jump(GOTO, Label(29)), + Label(26), Op(IRETURN), - Label(35), - Jump(GOTO, Label(6)), - Op(ATHROW), + Label(29), + Jump(GOTO, Label(0)), + Op(NOP), Op(ATHROW), )) @@ -1015,39 +1009,35 @@ class TestBCode extends DottyBytecodeTest { val sumMeth = getMethod(intListClass, "sum") assertSameCode(sumMeth, List( + Label(0), VarOp(ALOAD, 0), - VarOp(ASTORE, 2), - VarOp(ILOAD, 1), - VarOp(ISTORE, 3), - Label(4), - VarOp(ALOAD, 2), Field(GETFIELD, "IntList", "tail", "LIntList;"), - VarOp(ASTORE, 4), - VarOp(ALOAD, 4), - Jump(IFNONNULL, Label(16)), - VarOp(ILOAD, 3), + VarOp(ASTORE, 2), VarOp(ALOAD, 2), + Jump(IFNONNULL, Label(12)), + VarOp(ILOAD, 1), + VarOp(ALOAD, 0), Field(GETFIELD, "IntList", "head", "I"), Op(IADD), - Jump(GOTO, Label(30)), - Label(16), - VarOp(ALOAD, 4), - VarOp(ASTORE, 5), - VarOp(ILOAD, 3), + Jump(GOTO, Label(26)), + Label(12), VarOp(ALOAD, 2), + VarOp(ASTORE, 3), + VarOp(ILOAD, 1), + VarOp(ALOAD, 0), Field(GETFIELD, "IntList", "head", "I"), Op(IADD), - VarOp(ISTORE, 6), - VarOp(ALOAD, 5), - VarOp(ASTORE, 2), - VarOp(ILOAD, 6), - VarOp(ISTORE, 3), - Jump(GOTO, Label(33)), - Label(30), + VarOp(ISTORE, 4), + VarOp(ALOAD, 3), + VarOp(ASTORE, 0), + VarOp(ILOAD, 4), + VarOp(ISTORE, 1), + Jump(GOTO, Label(29)), + Label(26), Op(IRETURN), - Label(33), - Jump(GOTO, Label(4)), - Op(ATHROW), + Label(29), + Jump(GOTO, Label(0)), + Op(NOP), Op(ATHROW), )) }