JVM_IR: make primitive == object slightly less lazy.

Discarding the value used to leave an unused-but-never-destroyed temporary variable. It's best to not separate calls to `enterTemp` and `leaveTemp`. Not sure what kind of test to add though, since this is minor -- if the result of the comparison is discarded, then the entire statement is more or less pointless. #KT-42251 Fixed
2020-10-05 14:05:19 +02:00
parent 4c09906235
commit a9b53adc50
6 changed files with 64 additions and 60 deletions
@@ -1765,6 +1765,11 @@ public class FirBytecodeTextTestGenerated extends AbstractFirBytecodeTextTest {
        public void testSimpleConstructorNotRedundantNotOptimizable() throws Exception {
            runTest("compiler/testData/codegen/bytecodeText/deadCodeElimination/simpleConstructorNotRedundantNotOptimizable.kt");
        }
+
+        @TestMetadata("unusedPrimitiveAndObjectEquals.kt")
+        public void testUnusedPrimitiveAndObjectEquals() throws Exception {
+            runTest("compiler/testData/codegen/bytecodeText/deadCodeElimination/unusedPrimitiveAndObjectEquals.kt");
+        }
    }

    @TestMetadata("compiler/testData/codegen/bytecodeText/defaultArguments")
@@ -135,49 +135,53 @@ class NonIEEE754FloatComparison(val op: IElementType, val a: MaterialValue, val
 }

 class PrimitiveToObjectComparison(
-    val op: IElementType,
-    private val boxedValue: MaterialValue,
-    private val useNullCheck: Boolean,
-    private val primitiveType: Type,
-    private val loadOther: () -> MaterialValue,
-) : BooleanValue(boxedValue.codegen) {
-
-    override fun jumpIfFalse(target: Label) {
+    private val op: IElementType,
+    private val leftIsPrimitive: Boolean,
+    private val left: MaterialValue,
+    private val right: MaterialValue
+) : BooleanValue(left.codegen) {
+    private fun checkTypeAndCompare(onWrongType: Label): BooleanValue {
        val compareLabel = Label()
+        // If it's the left value that needs unboxing, it should be moved to the top of the stack. `AsmUtil.swap`
+        // is theoretically OK, but in practice breaks peephole optimization passes that unbox longs/doubles,
+        // so just storing in a variable is safer.
+        val tmp = if (leftIsPrimitive) -1 else codegen.frameMap.enterTemp(right.type).also { mv.store(it, right.type) }
        mv.dup()
-        if (useNullCheck) {
+        if (AsmUtil.isBoxedPrimitiveType(if (leftIsPrimitive) right.type else left.type)) {
            mv.ifnonnull(compareLabel)
        } else {
-            mv.instanceOf(AsmUtil.boxType(primitiveType))
+            mv.instanceOf(AsmUtil.boxType(if (leftIsPrimitive) left.type else right.type))
            mv.ifne(compareLabel)
        }
-        mv.pop()
-        mv.goTo(target)
+        // Type checking of the object failed, values are irrelevant now:
+        if (leftIsPrimitive) right.discard() // else it's already popped by `mv.store`
+        left.discard()
+        mv.goTo(onWrongType)
        mv.mark(compareLabel)
-        val unboxedValue = boxedValue.materializedAt(primitiveType, boxedValue.irType)
-        BooleanComparison(op, unboxedValue, loadOther()).jumpIfFalse(target)
+        // Type checking OK, can unbox and compare:
+        return if (leftIsPrimitive) {
+            BooleanComparison(op, left, right.materializedAt(left.type, right.irType))
+        } else {
+            val leftUnboxed = left.materializedAt(right.type, left.irType)
+            mv.load(tmp, right.type)
+            codegen.frameMap.leaveTemp(right.type)
+            BooleanComparison(op, leftUnboxed, right)
+        }
+    }
+
+    override fun jumpIfFalse(target: Label) {
+        checkTypeAndCompare(target).jumpIfFalse(target)
    }

    override fun jumpIfTrue(target: Label) {
-        val compareLabel = Label()
-        val endLabel = Label()
-        mv.dup()
-        if (useNullCheck) {
-            mv.ifnonnull(compareLabel)
-        } else {
-            mv.instanceOf(AsmUtil.boxType(primitiveType))
-            mv.ifne(compareLabel)
-        }
-        mv.pop()
-        mv.goTo(endLabel)
-        mv.mark(compareLabel)
-        val unboxedValue = boxedValue.materializedAt(primitiveType, boxedValue.irType)
-        BooleanComparison(op, unboxedValue, loadOther()).jumpIfTrue(target)
-        mv.mark(endLabel)
+        val wrongType = Label()
+        checkTypeAndCompare(wrongType).jumpIfTrue(target)
+        mv.mark(wrongType)
    }

    override fun discard() {
-        boxedValue.discard()
+        right.discard()
+        left.discard()
    }
 }

@@ -16,7 +16,6 @@ import org.jetbrains.kotlin.codegen.StackValue
 import org.jetbrains.kotlin.codegen.intrinsics.IntrinsicMethods
 import org.jetbrains.kotlin.config.LanguageFeature
 import org.jetbrains.kotlin.ir.descriptors.toIrBasedKotlinType
-import org.jetbrains.kotlin.ir.expressions.IrExpression
 import org.jetbrains.kotlin.ir.expressions.IrFunctionAccessExpression
 import org.jetbrains.kotlin.ir.expressions.IrStatementOrigin
 import org.jetbrains.kotlin.ir.types.classOrNull
@@ -26,7 +25,6 @@ import org.jetbrains.kotlin.ir.util.isEnumEntry
 import org.jetbrains.kotlin.ir.util.isIntegerConst
 import org.jetbrains.kotlin.ir.util.isNullConst
 import org.jetbrains.kotlin.lexer.KtTokens
-import org.jetbrains.kotlin.psi2ir.generators.hasNoSideEffects
 import org.jetbrains.kotlin.resolve.jvm.AsmTypes
 import org.jetbrains.kotlin.resolve.jvm.jvmSignature.JvmMethodSignature
 import org.jetbrains.kotlin.types.isNullable
@@ -83,21 +81,6 @@ class Equals(val operator: IElementType) : IntrinsicMethod() {
        val rightType = with(codegen) { b.asmType }
        val opToken = expression.origin

-        fun loadOther(expression: IrExpression, type: Type): () -> MaterialValue {
-            return if (expression.hasNoSideEffects()) {
-                { expression.accept(codegen, data).materializedAt(type, expression.type) }
-            } else {
-                val aValue = expression.accept(codegen, data).materializedAt(type, expression.type)
-                val local = codegen.frameMap.enterTemp(type)
-                codegen.mv.store(local, type)
-                ({
-                    codegen.mv.load(local, type)
-                    codegen.frameMap.leaveTemp(type)
-                    aValue
-                })
-            }
-        }
-
        // Avoid boxing for `primitive == object` and `boxed primitive == primitive` where we know
        // what comparison means. The optimization does not apply to `object == primitive` as equals
        // could be overridden for the object.
@@ -105,19 +88,9 @@ class Equals(val operator: IElementType) : IntrinsicMethod() {
            ((AsmUtil.isIntOrLongPrimitive(leftType) && !AsmUtil.isPrimitive(rightType)) ||
                    (AsmUtil.isIntOrLongPrimitive(rightType) && AsmUtil.isBoxedPrimitiveType(leftType)))
        ) {
-            val leftIsPrimitive = AsmUtil.isIntOrLongPrimitive(leftType)
-            val primitiveType = if (leftIsPrimitive) leftType else rightType
-            val nonPrimitiveType = if (leftIsPrimitive) rightType else leftType
-            val useNullCheck = AsmUtil.isBoxedPrimitiveType(nonPrimitiveType)
-            return if (leftIsPrimitive) {
-                val loadOther = loadOther(a, leftType)
-                val boxedValue = b.accept(codegen, data).materializedAt(rightType, b.type)
-                PrimitiveToObjectComparison(operator, boxedValue, useNullCheck, primitiveType, loadOther)
-            } else {
-                val boxedValue = a.accept(codegen, data).materializedAt(leftType, a.type)
-                val loadOther = loadOther(b, rightType)
-                PrimitiveToObjectComparison(operator, boxedValue, useNullCheck, primitiveType, loadOther)
-            }
+            val aValue = a.accept(codegen, data).materializedAt(leftType, a.type)
+            val bValue = b.accept(codegen, data).materializedAt(rightType, b.type)
+            return PrimitiveToObjectComparison(operator, AsmUtil.isIntOrLongPrimitive(leftType), aValue, bValue)
        }

        val aIsEnum = a.type.classOrNull?.owner?.run { isEnumClass || isEnumEntry } == true
@@ -0,0 +1,12 @@
+var x = 1
+
+fun box() {
+    val y: Number = 1
+    x == y
+}
+
+// 0 equals
+// JVM_TEMPLATES
+// 1 ICMP
+// JVM_IR_TEMPLATES
+// 0 ICMP
@@ -1810,6 +1810,11 @@ public class BytecodeTextTestGenerated extends AbstractBytecodeTextTest {
        public void testSimpleConstructorNotRedundantNotOptimizable() throws Exception {
            runTest("compiler/testData/codegen/bytecodeText/deadCodeElimination/simpleConstructorNotRedundantNotOptimizable.kt");
        }
+
+        @TestMetadata("unusedPrimitiveAndObjectEquals.kt")
+        public void testUnusedPrimitiveAndObjectEquals() throws Exception {
+            runTest("compiler/testData/codegen/bytecodeText/deadCodeElimination/unusedPrimitiveAndObjectEquals.kt");
+        }
    }

    @TestMetadata("compiler/testData/codegen/bytecodeText/defaultArguments")
@@ -1765,6 +1765,11 @@ public class IrBytecodeTextTestGenerated extends AbstractIrBytecodeTextTest {
        public void testSimpleConstructorNotRedundantNotOptimizable() throws Exception {
            runTest("compiler/testData/codegen/bytecodeText/deadCodeElimination/simpleConstructorNotRedundantNotOptimizable.kt");
        }
+
+        @TestMetadata("unusedPrimitiveAndObjectEquals.kt")
+        public void testUnusedPrimitiveAndObjectEquals() throws Exception {
+            runTest("compiler/testData/codegen/bytecodeText/deadCodeElimination/unusedPrimitiveAndObjectEquals.kt");
+        }
    }

    @TestMetadata("compiler/testData/codegen/bytecodeText/defaultArguments")