JVM_IR: handle Nothing and Unit more consistently.

* In blocks, discard the result of any statement that has a return type other than void. This was previously done by wrapping each statement into an "implicit Unit conversion" that was actually compiled down to a stack pop instead. If an expression happened to already have type Unit, however, such a conversion was not inserted, resulting in a stray reference on the stack. These conversions are now redundant and should probably be removed. * In assignments and non-exhaustive conditionals, materialize a Unit on the stack to avoid depth mismatches that trip up the bytecode validator. Because such expressions are generally used at block level (and, indeed, the frontend will reject a non-exhaustive conditional used as an expression), combined with the above change this results in no additional GETSTATIC opcodes, as they are immediately removed by the peephole optimizer.
2019-03-26 12:20:10 +01:00
parent bdad3cace9
commit ef5e02da84
19 changed files with 101 additions and 124 deletions
@@ -49,6 +49,8 @@ import org.jetbrains.kotlin.resolve.jvm.AsmTypes.OBJECT_TYPE
 import org.jetbrains.kotlin.synthetic.SyntheticJavaPropertyDescriptor
 import org.jetbrains.kotlin.types.KotlinType
 import org.jetbrains.kotlin.types.TypeUtils
+import org.jetbrains.kotlin.types.typeUtil.isNothing
+import org.jetbrains.kotlin.types.typeUtil.isUnit
 import org.jetbrains.kotlin.types.typesApproximation.approximateCapturedTypes
 import org.jetbrains.kotlin.types.upperIfFlexible
 import org.jetbrains.kotlin.utils.addToStdlib.firstIsInstanceOrNull
@@ -127,16 +129,13 @@ class ExpressionCodegen(
        val result = irFunction.body!!.accept(this, info)
        markFunctionLineNumber()
        val returnType = typeMapper.mapReturnType(irFunction.descriptor)
-        if (irFunction.body is IrExpressionBody) {
+        val body = irFunction.body!!
+        // If this function has an expression body, return the result of that expression.
+        // Otherwise, if it does not end in a return statement, it must be void-returning,
+        // and an explicit return instruction at the end is still required to pass validation.
+        if (body !is IrStatementContainer || body.statements.lastOrNull() !is IrReturn) {
+            coerce(result.type, returnType, mv)
            mv.areturn(returnType)
-            //TODO merge branch inside next one
-        } else if (!endsWithReturn(irFunction.body!!)) {
-            if (returnType == Type.VOID_TYPE) {
-                mv.areturn(returnType)
-            } else {
-                StackValue.none().put(returnType, null, mv)
-                mv.areturn(returnType)
-            }
        }
        writeLocalVariablesInTable(info)
        writeParameterInLocalVariableTable(startLabel)
@@ -194,19 +193,9 @@ class ExpressionCodegen(
        )
    }

-    private fun endsWithReturn(body: IrBody): Boolean {
-        val lastStatement = if (body is IrStatementContainer) {
-            body.statements.lastOrNull() ?: body
-        } else body
-        return lastStatement is IrReturn
-    }
-
-    override fun visitBlockBody(body: IrBlockBody, data: BlockInfo): StackValue {
-        return body.statements.fold(none()) { _, exp ->
-            exp.accept(this, data).also {
-                (exp as? IrExpression)?.markEndOfStatementIfNeeded()
-            }
-        }
+    private fun StackValue.discard(): StackValue {
+        coerce(type, Type.VOID_TYPE, mv)
+        return none()
    }

    override fun visitBlock(expression: IrBlock, data: BlockInfo): StackValue {
@@ -244,13 +233,21 @@ class ExpressionCodegen(
        }
    }

-    override fun visitContainerExpression(expression: IrContainerExpression, data: BlockInfo): StackValue {
-        val result = expression.statements.fold(none()) { _, exp ->
-            //coerceNotToUnit(r.type, Type.VOID_TYPE)
-            exp.accept(this, data)
+    private fun visitStatementContainer(container: IrStatementContainer, data: BlockInfo): StackValue {
+        return container.statements.fold(none()) { prev, exp ->
+            prev.discard()
+            gen(exp, data).also {
+                (exp as? IrExpression)?.markEndOfStatementIfNeeded()
+            }
        }
-        // Blocks with nothing type do not generate a value on the stack.
-        if (expression.type.isNothing()) return none()
+    }
+
+    override fun visitBlockBody(body: IrBlockBody, data: BlockInfo): StackValue {
+        return visitStatementContainer(body, data).discard()
+    }
+
+    override fun visitContainerExpression(expression: IrContainerExpression, data: BlockInfo): StackValue {
+        val result = visitStatementContainer(expression, data)
        return coerceNotToUnit(result.type, result.kotlinType, expression.type.toKotlinType())
    }

@@ -458,19 +455,21 @@ class ExpressionCodegen(

    override fun visitSetField(expression: IrSetField, data: BlockInfo): StackValue {
        val expressionValue = expression.value
-        if (irFunction is IrConstructor && irFunction.isPrimary) {
-            // Do not add redundant field initializers that initialize to default values.
-            // "expression.origin == null" means that the field is initialized when it is declared,
-            // i.e., not in an initializer block or constructor body.
-            if (expression.origin == null && expressionValue is IrConst<*> && isDefaultValueForType(
-                    expression.symbol.owner.type, expressionValue
-                )
-            ) return none()
+        // Do not add redundant field initializers that initialize to default values.
+        // "expression.origin == null" means that the field is initialized when it is declared,
+        // i.e., not in an initializer block or constructor body.
+        val skip = irFunction is IrConstructor && irFunction.isPrimary &&
+                expression.origin == null && expressionValue is IrConst<*> &&
+                isDefaultValueForType(expression.symbol.owner.type, expressionValue)
+        if (!skip) {
+            expression.markLineNumber(startOffset = true)
+            val fieldValue = generateFieldValue(expression, data)
+            fieldValue.store(expressionValue.accept(this, data), mv)
        }
-        expression.markLineNumber(startOffset = true)
-        val fieldValue = generateFieldValue(expression, data)
-        fieldValue.store(expressionValue.accept(this, data), mv)
-        return none()
+        // Assignments can be used as expressions, so return a value. Redundant pushes
+        // will be eliminated by the peephole optimizer.
+        putUnitInstance(mv)
+        return onStack(AsmTypes.UNIT_TYPE)
    }


@@ -538,7 +537,10 @@ class ExpressionCodegen(
        expression.value.markLineNumber(startOffset = true)
        val value = expression.value.accept(this, data)
        StackValue.local(findLocalIndex(expression.symbol), expression.descriptor.asmType).store(value, mv)
-        return none()
+        // Assignments can be used as expressions, so return a value. Redundant pushes
+        // will be eliminated by the peephole optimizer.
+        putUnitInstance(mv)
+        return onStack(AsmTypes.UNIT_TYPE)
    }

    override fun <T> visitConst(expression: IrConst<T>, data: BlockInfo): StackValue {
@@ -681,57 +683,46 @@ class ExpressionCodegen(

    override fun visitWhen(expression: IrWhen, data: BlockInfo): StackValue {
        expression.markLineNumber(startOffset = true)
-        val switch = SwitchGenerator(expression, data, this).generate()
-        return switch ?: genIfWithBranches(expression.branches[0], data, expression.type.toKotlinType(), expression.branches.drop(1))
-    }
+        SwitchGenerator(expression, data, this).generate()?.let { return it }

-    private fun genIfWithBranches(branch: IrBranch, data: BlockInfo, type: KotlinType, otherBranches: List<IrBranch>): StackValue {
-        // True or false conditions known at compile time need not be generated.
-        val shouldGenerateCondition = !branch.condition.isFalseConst() && !branch.condition.isTrueConst()
-        // Body of an always-false-condition need not be generated.
-        val shouldGenerateBody = !branch.condition.isFalseConst()
-        // Don't generate the tail if it doesn't exist or isn't reachable.
-        val shouldGenerateTail = !otherBranches.isEmpty() && !branch.condition.isTrueConst()
-
-        val elseLabel = Label()
+        val type = expression.type.toKotlinType()
        val endLabel = Label()
-
-        if (shouldGenerateCondition) {
-            genConditionalJumpWithOptimizationsIfPossible(branch.condition, data, elseLabel)
-        } else {
-            // Even when a condition isn't generated, a linenumber and nop is still required so that a debugger can break on the line of the
-            // condition, except for the explicit "else".
-            if (branch !is IrElseBranch) {
-                branch.condition.markLineNumber(startOffset = true)
-                mv.nop()
+        var exhaustive = false
+        for (branch in expression.branches) {
+            val elseLabel = Label()
+            if (branch.condition.isFalseConst() || branch.condition.isTrueConst()) {
+                // True or false conditions known at compile time need not be generated. A linenumber and nop are still required
+                // for a debugger to break on the line of the condition.
+                if (branch !is IrElseBranch) {
+                    branch.condition.markLineNumber(startOffset = true)
+                    mv.nop()
+                }
+                if (branch.condition.isFalseConst())
+                    continue // The branch body is dead code.
+            } else {
+                genConditionalJumpWithOptimizationsIfPossible(branch.condition, data, elseLabel)
            }
-        }
-
-        val resultFromBody = if (shouldGenerateBody) {
-            val thenBranch = branch.result
-            val result = thenBranch.run {
-                val stackValue = gen(this, data)
-                coerceNotToUnit(stackValue.type, stackValue.kotlinType, type)
+            gen(branch.result, data).let {
+                coerceNotToUnit(it.type, it.kotlinType, type)
+            }
+            if (branch.condition.isTrueConst()) {
+                exhaustive = true
+                break // The rest of the expression is dead code.
            }
            mv.goTo(endLabel)
            mv.mark(elseLabel)
-            result
-        } else {
-            none()
        }

-        val resultFromTail = if (shouldGenerateTail) {
-            val nextBranch = otherBranches.first()
-            genIfWithBranches(nextBranch, data, type, otherBranches.drop(1))
-        } else {
-            none()
+        if (!exhaustive) {
+            // TODO: make all non-exhaustive `if`/`when` return Nothing.
+            if (type.isUnit())
+                putUnitInstance(mv)
+            else if (!type.isNothing())
+                throw AssertionError("non-exhaustive `if`/`when` wants to return $type")
        }

-        // endLabel is only used to jump from end-of-then-body to the end of the whole if cascade.
-        if (shouldGenerateBody)
-            mv.mark(endLabel)
-
-        return if (shouldGenerateBody) resultFromBody else resultFromTail
+        mv.mark(endLabel)
+        return if (type.isNothing()) none() else expression.onStack
    }

    private fun genConditionalJumpWithOptimizationsIfPossible(
@@ -814,8 +805,7 @@ class ExpressionCodegen(
            IrTypeOperator.IMPLICIT_COERCION_TO_UNIT -> {
                val result = expression.argument.accept(this, data)
                expression.argument.markEndOfStatementIfNeeded()
-                coerce(result.type, Type.VOID_TYPE, mv)
-                return none()
+                return result.discard()
            }

            IrTypeOperator.IMPLICIT_CAST -> {
@@ -931,8 +921,8 @@ class ExpressionCodegen(

        with(LoopInfo(loop, continueLabel, endLabel)) {
            data.addInfo(this)
-            loop.body?.apply {
-                gen(this, data)
+            loop.body?.let {
+                gen(it, data).discard()
            }
            data.removeInfo(this)
        }
@@ -991,8 +981,8 @@ class ExpressionCodegen(

        with(LoopInfo(loop, continueLabel, endLabel)) {
            data.addInfo(this)
-            loop.body?.apply {
-                gen(this, data)
+            loop.body?.let {
+                gen(it, data).discard()
            }
            data.removeInfo(this)
        }
@@ -1229,7 +1219,8 @@ class ExpressionCodegen(

    internal fun coerceNotToUnit(fromType: Type, fromKotlinType: KotlinType?, toKotlinType: KotlinType): StackValue {
        val asmToType = toKotlinType.asmType
-        if (asmToType != AsmTypes.UNIT_TYPE || TypeUtils.isNullableType(toKotlinType)) {
+        // A void should still be materialized as a Unit to avoid stack depth mismatches.
+        if (asmToType != AsmTypes.UNIT_TYPE || fromType == Type.VOID_TYPE || TypeUtils.isNullableType(toKotlinType)) {
            coerce(fromType, fromKotlinType, asmToType, toKotlinType, mv)
            return onStack(asmToType, toKotlinType)
        }
@@ -28,8 +28,6 @@ class SwitchGenerator(private val expression: IrWhen, private val data: BlockInf

    // @return null if the IrWhen cannot be emitted as lookupswitch or tableswitch.
    fun generate(): StackValue? {
-        val endLabel = Label()
-        var defaultLabel = endLabel
        val expressionToLabels = ArrayList<ExpressionToLabel>()
        var elseExpression: IrExpression? = null
        val callToLabels = ArrayList<CallToLabel>()
@@ -38,7 +36,6 @@ class SwitchGenerator(private val expression: IrWhen, private val data: BlockInf
        for (branch in expression.branches) {
            if (branch is IrElseBranch) {
                elseExpression = branch.result
-                defaultLabel = Label()
            } else {
                val conditions = matchConditions(branch.condition) ?: return null
                val thenLabel = Label()
@@ -73,8 +70,6 @@ class SwitchGenerator(private val expression: IrWhen, private val data: BlockInf
            areConstIntComparisons(calls) ->
                IntSwitch(
                    subject,
-                    defaultLabel,
-                    endLabel,
                    elseExpression,
                    expressionToLabels,
                    cases
@@ -82,8 +77,6 @@ class SwitchGenerator(private val expression: IrWhen, private val data: BlockInf
            areConstStringComparisons(calls) ->
                StringSwitch(
                    subject,
-                    defaultLabel,
-                    endLabel,
                    elseExpression,
                    expressionToLabels,
                    cases
@@ -197,11 +190,12 @@ class SwitchGenerator(private val expression: IrWhen, private val data: BlockInf

    abstract inner class Switch(
        val subject: IrGetValue,
-        val defaultLabel: Label,
-        val endLabel: Label,
        val elseExpression: IrExpression?,
        val expressionToLabels: ArrayList<ExpressionToLabel>
    ) {
+        protected val endLabel = Label()
+        protected val defaultLabel = Label()
+
        open fun shouldOptimize() = false

        open fun genOptimizedIfEnoughCases(): StackValue? {
@@ -251,14 +245,13 @@ class SwitchGenerator(private val expression: IrWhen, private val data: BlockInf
        }

        protected fun genElseExpression(): StackValue {
+            mv.visitLabel(defaultLabel)
            return if (elseExpression == null) {
-                // There's no else part. No stack value will be generated.
-                StackValue.putUnitInstance(mv)
-                onStack(Type.VOID_TYPE)
+                // There's no else part. Generate Unit if needed.
+                coerceNotToUnit(Type.VOID_TYPE, null, expression.type.toKotlinType())
            } else {
                // Generate the else part.
-                mv.visitLabel(defaultLabel)
-                val stackValue = elseExpression.run { gen(this, data) }
+                val stackValue = gen(elseExpression, data)
                coerceNotToUnit(stackValue.type, stackValue.kotlinType, expression.type.toKotlinType())
            }
        }
@@ -266,12 +259,10 @@ class SwitchGenerator(private val expression: IrWhen, private val data: BlockInf

    inner class IntSwitch(
        subject: IrGetValue,
-        defaultLabel: Label,
-        endLabel: Label,
        elseExpression: IrExpression?,
        expressionToLabels: ArrayList<ExpressionToLabel>,
        private val cases: List<ValueToLabel>
-    ) : Switch(subject, defaultLabel, endLabel, elseExpression, expressionToLabels) {
+    ) : Switch(subject, elseExpression, expressionToLabels) {

        // IF is more compact when there are only 1 or fewer branches, in addition to else.
        override fun shouldOptimize() = cases.size > 1
@@ -326,12 +317,10 @@ class SwitchGenerator(private val expression: IrWhen, private val data: BlockInf

    inner class StringSwitch(
        subject: IrGetValue,
-        defaultLabel: Label,
-        endLabel: Label,
        elseExpression: IrExpression?,
        expressionToLabels: ArrayList<ExpressionToLabel>,
        private val cases: List<ValueToLabel>
-    ) : Switch(subject, defaultLabel, endLabel, elseExpression, expressionToLabels) {
+    ) : Switch(subject, elseExpression, expressionToLabels) {

        private val hashToStringAndExprLabels = HashMap<Int, ArrayList<ValueToLabel>>()
        private val hashAndSwitchLabels = ArrayList<ValueToLabel>()
@@ -1,4 +1,3 @@
-// IGNORE_BACKEND: JVM_IR
 // WITH_RUNTIME

 fun box(): String {
@@ -1,3 +1,7 @@
+// Even before any IR lowerings, the type of `when` is determined to be
+// Unit even though the outer `if` still returns `Int?`. This results
+// in a ClassCastException when that Unit is converted into a Number.
+// IGNORE_BACKEND: JVM_IR
 fun test(
        b: Boolean,
        i: Int
@@ -1,4 +1,3 @@
-// IGNORE_BACKEND: JVM_IR
 interface Callable {
    fun call(b: Boolean)
 }
@@ -1,4 +1,3 @@
-// IGNORE_BACKEND: JVM_IR
 class A(val p: String) {
    val prop: String = throw RuntimeException()
 }
@@ -1,4 +1,3 @@
-// IGNORE_BACKEND: JVM_IR
 // TARGET_BACKEND: JVM
 // WITH_REFLECT

@@ -1,4 +1,3 @@
-// IGNORE_BACKEND: JVM_IR
 // CHECK_CASES_COUNT: function=test1 count=2
 // CHECK_IF_COUNT: function=test1 count=0
 // CHECK_BREAKS_COUNT: function=test1 count=1
@@ -1,4 +1,3 @@
-// IGNORE_BACKEND: JVM_IR
 // NO_CHECK_LAMBDA_INLINING
 // FILE: 1.kt
 // WITH_RUNTIME
@@ -1,4 +1,3 @@
-// IGNORE_BACKEND: JVM_IR
 // FILE: 1.kt
 // WITH_RUNTIME
 package test
@@ -1,4 +1,3 @@
-// IGNORE_BACKEND: JVM_IR
 // FILE: 1.kt
 // WITH_RUNTIME
 package test
@@ -1,4 +1,3 @@
-// IGNORE_BACKEND: JVM_IR
 // FILE: 1.kt
 // WITH_RUNTIME
 package test
@@ -1,4 +1,3 @@
-// IGNORE_BACKEND: JVM_IR
 // FILE: 1.kt

 package test
@@ -1,4 +1,3 @@
-// IGNORE_BACKEND: JVM_IR
 // FILE: 1.kt

 package test
@@ -1,4 +1,3 @@
-// IGNORE_BACKEND: JVM_IR
 // FILE: 1.kt
 package test

@@ -1,4 +1,3 @@
-// IGNORE_BACKEND: JVM_IR
 // PROPERTY_NOT_USED: p1
 // PROPERTY_NOT_READ_FROM: p2
 // PROPERTY_NOT_WRITTEN_TO: p3
@@ -18,3 +18,6 @@ inline fun inlineCall(predicate: (String?) -> Boolean): Boolean {
 // 0 LINENUMBER 7
 // 0 LINENUMBER 8
 // 1 LINENUMBER 9
+
+// Not actually inlined, so there is a LINENUMBER 7 because the if's body is not considered dead.
+// IGNORE_BACKEND: JVM_IR
@@ -1,4 +1,3 @@
-// IGNORE_BACKEND: JVM_IR
 fun z() {}

 fun foo() {
@@ -12,4 +12,8 @@ fun foo(x: Int) {
    }
 }

-// 2 3 4 5 6 +8 9 10 11 8 13
+// 2 3 4 5 6 +8 9 10 11 8 13
+
+// JVM_IR also generates a LINENUMBER 12, which seems consistent with the fact that
+// there is a LINENUMBER 6, but still fails the test.
+// IGNORE_BACKEND: JVM_IR