From fb6eafddf1085fffb2eaf85b2f07d0da28c6e094 Mon Sep 17 00:00:00 2001 From: Mads Ager Date: Mon, 21 Jan 2019 08:15:24 +0100 Subject: [PATCH] JVM_IR: Generate better code for null checks. Simplify ifs when branches have condition true/false. Simplify blocks containing only a variable declaration and a variable get of the same variable. Simplify to just the condition. Do not introduce temporary variables for constants for null checks. Constants have no side-effects and can be reloaded freely instead of going through a local. This simplifies code such as "42.toLong()!!" so that the resulting code has no branches and uses no locals. The simplifications happen as follows: ``` block temp = 42.toLong() when (temp == null) throw NPE (true) load temp ---> null test simplification block temp = 42.toLong() when (false) throw NPE (true) load temp ---> when simplification block temp = 42.toLong() load temp ---> block simplification 42.toLong() ``` --- .../lower/JvmBuiltinOptimizationLowering.kt | 136 ++++++++++++++++-- .../org/jetbrains/kotlin/ir/util/IrUtils.kt | 3 + .../testData/codegen/box/classes/kt496.kt | 1 - .../tryCatchInExpressions/splitTryCorner1.kt | 1 - .../box/deadCodeElimination/kt14357.kt | 1 - .../codegen/box/finally/finallyAndFinally.kt | 1 - .../exceptionTable/nestedWithReturnsSimple.kt | 1 - .../deadCodeElimination/kt14357.kt | 1 - .../bytecodeText/exclExcl/primitive.kt | 9 ++ .../codegen/bytecodeText/when/whenNull.kt | 1 - .../lineNumber/custom/primitiveNullChecks.kt | 7 + .../codegen/BytecodeTextTestGenerated.java | 18 +++ .../codegen/IrBytecodeTextTestGenerated.java | 18 +++ .../codegen/LineNumberTestGenerated.java | 5 + .../codegen/ir/IrLineNumberTestGenerated.java | 5 + 15 files changed, 192 insertions(+), 16 deletions(-) create mode 100644 compiler/testData/codegen/bytecodeText/exclExcl/primitive.kt create mode 100644 compiler/testData/lineNumber/custom/primitiveNullChecks.kt diff --git a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/JvmBuiltinOptimizationLowering.kt b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/JvmBuiltinOptimizationLowering.kt index 564c4741cea..e9387130c88 100644 --- a/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/JvmBuiltinOptimizationLowering.kt +++ b/compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/lower/JvmBuiltinOptimizationLowering.kt @@ -8,17 +8,19 @@ package org.jetbrains.kotlin.backend.jvm.lower import org.jetbrains.kotlin.backend.common.FileLoweringPass import org.jetbrains.kotlin.backend.jvm.JvmBackendContext import org.jetbrains.kotlin.codegen.intrinsics.Not +import org.jetbrains.kotlin.ir.IrStatement +import org.jetbrains.kotlin.ir.declarations.IrDeclarationOrigin import org.jetbrains.kotlin.ir.declarations.IrFile -import org.jetbrains.kotlin.ir.expressions.IrCall -import org.jetbrains.kotlin.ir.expressions.IrConst -import org.jetbrains.kotlin.ir.expressions.IrExpression -import org.jetbrains.kotlin.ir.expressions.IrGetValue +import org.jetbrains.kotlin.ir.declarations.IrVariable +import org.jetbrains.kotlin.ir.expressions.* import org.jetbrains.kotlin.ir.expressions.impl.IrBlockImpl import org.jetbrains.kotlin.ir.expressions.impl.IrConstImpl import org.jetbrains.kotlin.ir.types.isPrimitiveType import org.jetbrains.kotlin.ir.types.toKotlinType import org.jetbrains.kotlin.ir.util.coerceToUnitIfNeeded +import org.jetbrains.kotlin.ir.util.isFalseConst import org.jetbrains.kotlin.ir.util.isNullConst +import org.jetbrains.kotlin.ir.util.isTrueConst import org.jetbrains.kotlin.ir.visitors.IrElementTransformerVoid import org.jetbrains.kotlin.ir.visitors.transformChildrenVoid @@ -57,11 +59,12 @@ class JvmBuiltinOptimizationLowering(val context: JvmBackendContext) : FileLower return false } - private fun isNullCheckOfNullConstant(call: IrCall, context: JvmBackendContext): Boolean { + private fun isNullCheckOfConstant(call: IrCall, context: JvmBackendContext): Boolean { if (call.symbol == context.irBuiltIns.eqeqSymbol) { val left = call.getValueArgument(0)!! val right = call.getValueArgument(1)!! - return right.isNullConst() && left.isNullConst() + return (right.isNullConst() && left is IrConst<*>) + || (left.isNullConst() && right is IrConst<*>) } return false } @@ -87,12 +90,127 @@ class JvmBuiltinOptimizationLowering(val context: JvmBackendContext) : FileLower statements.add(constFalse) } } - } else if (isNullCheckOfNullConstant(expression, context)) { - IrConstImpl.constTrue(expression.startOffset, expression.endOffset, context.irBuiltIns.booleanType) + } else if (isNullCheckOfConstant(expression, context)) { + if (expression.getValueArgument(0)!!.isNullConst() && expression.getValueArgument(1)!!.isNullConst()) { + IrConstImpl.constTrue(expression.startOffset, expression.endOffset, context.irBuiltIns.booleanType) + } else { + IrConstImpl.constFalse(expression.startOffset, expression.endOffset, context.irBuiltIns.booleanType) + } } else { expression } } + + override fun visitWhen(expression: IrWhen): IrExpression { + expression.transformChildrenVoid(this) + // Remove all branches with constant false condition. + expression.branches.removeIf() { + it.condition.isFalseConst() + } + // If the only condition that is left has a constant true condition remove the + // when in favor of the result. If there are no conditions left, remove the when + // entirely and replace it with an empty block. + return if (expression.branches.size == 0) { + IrBlockImpl(expression.startOffset, expression.endOffset, context.irBuiltIns.unitType) + } else { + expression.branches.first().takeIf { it.condition.isTrueConst() }?.result ?: expression + } + } + + private fun isImmutableTemporaryVariableWithConstantValue(statement: IrStatement): Boolean { + return statement is IrVariable && + statement.origin == IrDeclarationOrigin.IR_TEMPORARY_VARIABLE && + !statement.isVar && + statement.initializer is IrConst<*> + } + + override fun visitBlock(expression: IrBlock): IrExpression { + expression.transformChildrenVoid(this) + // Remove declarations of immutable temporary variables with constant values. + // IrGetValue operations for such temporary variables are replaced + // by the initializer IrConst. This makes sure that we do not load and + // store constants in/from locals. For example + // + // "StringConstant"!! + // + // introduces a temporary variable for the string constant and generates + // a null check + // + // block + // temp = "StringConstant" + // when (eq(temp, null)) + // (true) -> throwNpe() + // (false) -> temp + // + // When generating code, this stores the string constant in a local and loads + // it from there. The removal of the temporary and the replacement of the loads + // of the temporary (see visitGetValue) with the constant avoid generating local + // loads and stores by turning this into + // + // block + // when (eq("StringConstant", null)) + // (true) -> throwNpe() + // (false) -> "StringConstant" + // + // which allows the equality check to be simplified away and we end up with + // just a const string load. + expression.statements.removeIf { + isImmutableTemporaryVariableWithConstantValue(it) + } + // Remove a block that contains only two statements: the declaration of a temporary + // variable and a load of the value of that temporary variable with just the initializer + // for the temporary variable. We only perform this transformation for compiler generated + // temporary variables. Local variables can be changed at runtime and therefore eliminating + // an actual local variable changes debugging behavior. + // + // This helps avoid temporary variables even for side-effecting expressions when they are + // not needed. Having a temporary variable leads to local loads and stores in the + // generated java bytecode which are not necessary. For example + // + // 42.toLong()!! + // + // introduces a temporary variable for the toLong() call and a null check + // block + // temp = 42.toLong() + // when (eq(temp, null)) + // (true) -> throwNep() + // (false) -> temp + // + // the when is simplified because long is a primitive type, which leaves us with + // + // block + // temp = 42.toLong() + // temp + // + // which can be simplified to simply + // + // block + // 42.toLong() + // + // Doing so we avoid local loads and stores. + if (expression.statements.size == 2) { + val first = expression.statements[0] + val second = expression.statements[1] + if (first is IrVariable + && first.origin == IrDeclarationOrigin.IR_TEMPORARY_VARIABLE + && second is IrGetValue + && first.symbol == second.symbol) { + expression.statements.clear() + first.initializer?.let { expression.statements.add(it) } + } + } + return expression + } + + override fun visitGetValue(expression: IrGetValue): IrExpression { + // Replace IrGetValue of an immutable temporary variable with a constant + // initializer with the constant initializer. + val variable = expression.symbol.owner + return if (isImmutableTemporaryVariableWithConstantValue(variable)) + (variable as IrVariable).initializer!! + else + expression + } }) } -} \ No newline at end of file +} diff --git a/compiler/ir/ir.tree/src/org/jetbrains/kotlin/ir/util/IrUtils.kt b/compiler/ir/ir.tree/src/org/jetbrains/kotlin/ir/util/IrUtils.kt index f8e2e80b677..ab6ffba1641 100644 --- a/compiler/ir/ir.tree/src/org/jetbrains/kotlin/ir/util/IrUtils.kt +++ b/compiler/ir/ir.tree/src/org/jetbrains/kotlin/ir/util/IrUtils.kt @@ -144,6 +144,9 @@ fun IrMemberAccessExpression.addArguments(args: List && this.kind == IrConstKind.Null +fun IrExpression.isTrueConst() = this is IrConst<*> && this.kind == IrConstKind.Boolean && this.value == true + +fun IrExpression.isFalseConst() = this is IrConst<*> && this.kind == IrConstKind.Boolean && this.value == false fun IrExpression.coerceToUnitIfNeeded(valueType: KotlinType, irBuiltIns: IrBuiltIns): IrExpression { return if (KotlinTypeChecker.DEFAULT.isSubtypeOf(valueType, irBuiltIns.unitType.toKotlinType())) diff --git a/compiler/testData/codegen/box/classes/kt496.kt b/compiler/testData/codegen/box/classes/kt496.kt index 6791f3714ca..e4c31c86a27 100644 --- a/compiler/testData/codegen/box/classes/kt496.kt +++ b/compiler/testData/codegen/box/classes/kt496.kt @@ -1,4 +1,3 @@ -// IGNORE_BACKEND: JVM_IR fun test1() : Boolean { try { return true diff --git a/compiler/testData/codegen/box/controlStructures/tryCatchInExpressions/splitTryCorner1.kt b/compiler/testData/codegen/box/controlStructures/tryCatchInExpressions/splitTryCorner1.kt index 9c1053f8fe0..bbf5ad07064 100644 --- a/compiler/testData/codegen/box/controlStructures/tryCatchInExpressions/splitTryCorner1.kt +++ b/compiler/testData/codegen/box/controlStructures/tryCatchInExpressions/splitTryCorner1.kt @@ -1,4 +1,3 @@ -// IGNORE_BACKEND: JVM_IR fun shouldReturnFalse() : Boolean { try { return true diff --git a/compiler/testData/codegen/box/deadCodeElimination/kt14357.kt b/compiler/testData/codegen/box/deadCodeElimination/kt14357.kt index 3ba1f3c08f7..377a6facdac 100644 --- a/compiler/testData/codegen/box/deadCodeElimination/kt14357.kt +++ b/compiler/testData/codegen/box/deadCodeElimination/kt14357.kt @@ -1,4 +1,3 @@ -// IGNORE_BACKEND: JVM_IR fun box(): String { if (false) { try { diff --git a/compiler/testData/codegen/box/finally/finallyAndFinally.kt b/compiler/testData/codegen/box/finally/finallyAndFinally.kt index e039302e374..e902bc8746f 100644 --- a/compiler/testData/codegen/box/finally/finallyAndFinally.kt +++ b/compiler/testData/codegen/box/finally/finallyAndFinally.kt @@ -1,4 +1,3 @@ -// IGNORE_BACKEND: JVM_IR class MyString { var s = "" operator fun plus(x : String) : MyString { diff --git a/compiler/testData/codegen/boxInline/nonLocalReturns/tryFinally/exceptionTable/nestedWithReturnsSimple.kt b/compiler/testData/codegen/boxInline/nonLocalReturns/tryFinally/exceptionTable/nestedWithReturnsSimple.kt index 61bd4dd98c4..91cf72d4f98 100644 --- a/compiler/testData/codegen/boxInline/nonLocalReturns/tryFinally/exceptionTable/nestedWithReturnsSimple.kt +++ b/compiler/testData/codegen/boxInline/nonLocalReturns/tryFinally/exceptionTable/nestedWithReturnsSimple.kt @@ -1,4 +1,3 @@ -// IGNORE_BACKEND: JVM_IR // FILE: 1.kt package test diff --git a/compiler/testData/codegen/bytecodeText/deadCodeElimination/kt14357.kt b/compiler/testData/codegen/bytecodeText/deadCodeElimination/kt14357.kt index fe679eed9ba..f81e9f51f5f 100644 --- a/compiler/testData/codegen/bytecodeText/deadCodeElimination/kt14357.kt +++ b/compiler/testData/codegen/bytecodeText/deadCodeElimination/kt14357.kt @@ -1,4 +1,3 @@ -// IGNORE_BACKEND: JVM_IR fun test() { if (false) { try { diff --git a/compiler/testData/codegen/bytecodeText/exclExcl/primitive.kt b/compiler/testData/codegen/bytecodeText/exclExcl/primitive.kt new file mode 100644 index 00000000000..6016b25aca3 --- /dev/null +++ b/compiler/testData/codegen/bytecodeText/exclExcl/primitive.kt @@ -0,0 +1,9 @@ +fun box(): String { + 42!! + 42.toLong()!! + return "OK"!! +} + +// 0 LOAD +// 0 STORE +// 0 IF diff --git a/compiler/testData/codegen/bytecodeText/when/whenNull.kt b/compiler/testData/codegen/bytecodeText/when/whenNull.kt index 8b1e4ab8ebc..19cc0a6acf8 100644 --- a/compiler/testData/codegen/bytecodeText/when/whenNull.kt +++ b/compiler/testData/codegen/bytecodeText/when/whenNull.kt @@ -1,4 +1,3 @@ -// IGNORE_BACKEND: JVM_IR fun test(a: Any?, b: Any?, c: Any?) { when (null) { a -> throw IllegalArgumentException("a is null") diff --git a/compiler/testData/lineNumber/custom/primitiveNullChecks.kt b/compiler/testData/lineNumber/custom/primitiveNullChecks.kt new file mode 100644 index 00000000000..a9073cf94e9 --- /dev/null +++ b/compiler/testData/lineNumber/custom/primitiveNullChecks.kt @@ -0,0 +1,7 @@ +fun box(): String { + 42!! + 42.toLong()!! + return "OK"!! +} + +// 2 3 4 \ No newline at end of file diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/BytecodeTextTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/BytecodeTextTestGenerated.java index 40dadad025d..712038c0417 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/BytecodeTextTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/BytecodeTextTestGenerated.java @@ -1459,6 +1459,24 @@ public class BytecodeTextTestGenerated extends AbstractBytecodeTextTest { } } + @TestMetadata("compiler/testData/codegen/bytecodeText/exclExcl") + @TestDataPath("$PROJECT_ROOT") + @RunWith(JUnit3RunnerWithInners.class) + public static class ExclExcl extends AbstractBytecodeTextTest { + private void runTest(String testDataFilePath) throws Exception { + KotlinTestUtils.runTest(this::doTest, TargetBackend.ANY, testDataFilePath); + } + + public void testAllFilesPresentInExclExcl() throws Exception { + KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("compiler/testData/codegen/bytecodeText/exclExcl"), Pattern.compile("^(.+)\\.kt$"), TargetBackend.ANY, true); + } + + @TestMetadata("primitive.kt") + public void testPrimitive() throws Exception { + runTest("compiler/testData/codegen/bytecodeText/exclExcl/primitive.kt"); + } + } + @TestMetadata("compiler/testData/codegen/bytecodeText/forLoop") @TestDataPath("$PROJECT_ROOT") @RunWith(JUnit3RunnerWithInners.class) diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/IrBytecodeTextTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/IrBytecodeTextTestGenerated.java index 53b9b73bd35..86e21b1219a 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/IrBytecodeTextTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/IrBytecodeTextTestGenerated.java @@ -1459,6 +1459,24 @@ public class IrBytecodeTextTestGenerated extends AbstractIrBytecodeTextTest { } } + @TestMetadata("compiler/testData/codegen/bytecodeText/exclExcl") + @TestDataPath("$PROJECT_ROOT") + @RunWith(JUnit3RunnerWithInners.class) + public static class ExclExcl extends AbstractIrBytecodeTextTest { + private void runTest(String testDataFilePath) throws Exception { + KotlinTestUtils.runTest(this::doTest, TargetBackend.JVM_IR, testDataFilePath); + } + + public void testAllFilesPresentInExclExcl() throws Exception { + KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("compiler/testData/codegen/bytecodeText/exclExcl"), Pattern.compile("^(.+)\\.kt$"), TargetBackend.JVM_IR, true); + } + + @TestMetadata("primitive.kt") + public void testPrimitive() throws Exception { + runTest("compiler/testData/codegen/bytecodeText/exclExcl/primitive.kt"); + } + } + @TestMetadata("compiler/testData/codegen/bytecodeText/forLoop") @TestDataPath("$PROJECT_ROOT") @RunWith(JUnit3RunnerWithInners.class) diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/LineNumberTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/LineNumberTestGenerated.java index 4dc72c6c894..3cc6acfec6e 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/LineNumberTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/LineNumberTestGenerated.java @@ -201,6 +201,11 @@ public class LineNumberTestGenerated extends AbstractLineNumberTest { runTest("compiler/testData/lineNumber/custom/noParametersArgumentCallInExpression.kt"); } + @TestMetadata("primitiveNullChecks.kt") + public void testPrimitiveNullChecks() throws Exception { + runTest("compiler/testData/lineNumber/custom/primitiveNullChecks.kt"); + } + @TestMetadata("smapInlineAsArgument.kt") public void testSmapInlineAsArgument() throws Exception { runTest("compiler/testData/lineNumber/custom/smapInlineAsArgument.kt"); diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrLineNumberTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrLineNumberTestGenerated.java index ff95c1f4226..823c562c95a 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrLineNumberTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/codegen/ir/IrLineNumberTestGenerated.java @@ -201,6 +201,11 @@ public class IrLineNumberTestGenerated extends AbstractIrLineNumberTest { runTest("compiler/testData/lineNumber/custom/noParametersArgumentCallInExpression.kt"); } + @TestMetadata("primitiveNullChecks.kt") + public void testPrimitiveNullChecks() throws Exception { + runTest("compiler/testData/lineNumber/custom/primitiveNullChecks.kt"); + } + @TestMetadata("smapInlineAsArgument.kt") public void testSmapInlineAsArgument() throws Exception { runTest("compiler/testData/lineNumber/custom/smapInlineAsArgument.kt");