From 271368ac53f517b97f6d73db43c8548b9ac4d421 Mon Sep 17 00:00:00 2001 From: Dmitry Petrov Date: Mon, 8 Nov 2021 14:40:35 +0300 Subject: [PATCH] JVM_IR prefer loop with inclusive bound for unsigned integer ranges In general, we would rather prefer a range-based for loop to look like a counter loop in Java ('for (i = start; i < end; ++i) { }'). This corresponds to i = start; do { if (i >= end) break; } while ( { ++i; true } ) However, HotSpot doesn't recognize Kotlin unsigned integer comparison in 'if (i >= end) break;' as a counter loop condition. Thus, the loop doesn't get optimized as a counter loop, resulting in a performance regression. If we use exclusive range-based for loop instead, then we actually use unsigned integer equality instead of unsigned integer comparison, which is Ok for HotSpot. KT-49444 --- .../codegen/FirBytecodeTextTestGenerated.java | 6 ++ .../common/lower/loops/HeaderProcessor.kt | 12 ++- .../lower/loops/handlers/DownToHandler.kt | 3 +- .../lower/loops/handlers/RangeToHandler.kt | 3 +- .../unsigned/forInConstBoundUnsignedRange.kt | 76 +++++++++++++++++++ .../codegen/BytecodeTextTestGenerated.java | 6 ++ .../codegen/IrBytecodeTextTestGenerated.java | 6 ++ 7 files changed, 108 insertions(+), 4 deletions(-) create mode 100644 compiler/testData/codegen/bytecodeText/forLoop/unsigned/forInConstBoundUnsignedRange.kt diff --git a/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBytecodeTextTestGenerated.java b/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBytecodeTextTestGenerated.java index cf47756f51f..d97b27224c0 100644 --- a/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBytecodeTextTestGenerated.java +++ b/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBytecodeTextTestGenerated.java @@ -3089,6 +3089,12 @@ public class FirBytecodeTextTestGenerated extends AbstractFirBytecodeTextTest { KtTestUtil.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("compiler/testData/codegen/bytecodeText/forLoop/unsigned"), Pattern.compile("^(.+)\\.kt$"), null, TargetBackend.JVM_IR, true); } + @Test + @TestMetadata("forInConstBoundUnsignedRange.kt") + public void testForInConstBoundUnsignedRange() throws Exception { + runTest("compiler/testData/codegen/bytecodeText/forLoop/unsigned/forInConstBoundUnsignedRange.kt"); + } + @Test @TestMetadata("forInDownToUIntMinValue.kt") public void testForInDownToUIntMinValue() throws Exception { diff --git a/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/loops/HeaderProcessor.kt b/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/loops/HeaderProcessor.kt index 36d1dd842ab..5a68ea0ed22 100644 --- a/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/loops/HeaderProcessor.kt +++ b/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/loops/HeaderProcessor.kt @@ -314,8 +314,11 @@ class ProgressionLoopHeader( override fun buildLoop(builder: DeclarationIrBuilder, oldLoop: IrLoop, newBody: IrExpression?) = with(builder) { - if (headerInfo.canOverflow) { - // If the induction variable CAN overflow, we cannot use it in the loop condition. Loop is lowered into something like: + if (headerInfo.canOverflow || + preferJavaLikeCounterLoop && headerInfo.progressionType is UnsignedProgressionType && headerInfo.isLastInclusive + ) { + // If the induction variable CAN overflow, we cannot use it in the loop condition. + // Loop is lowered into something like: // // if (inductionVar <= last) { // // Loop is not empty @@ -325,6 +328,11 @@ class ProgressionLoopHeader( // // Loop body // } while (loopVar != last) // } + // + // This loop form is also preferable for loops over unsigned progressions on JVM, + // because HotSpot doesn't recognize unsigned integer comparison as a counter loop condition. + // Unsigned integer equality is fine, though. + // See KT-49444 for performance comparison example. val newLoopOrigin = if (preferJavaLikeCounterLoop) this@ProgressionLoopHeader.context.doWhileCounterLoopOrigin else diff --git a/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/loops/handlers/DownToHandler.kt b/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/loops/handlers/DownToHandler.kt index a975a20e31d..e713addab83 100644 --- a/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/loops/handlers/DownToHandler.kt +++ b/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/loops/handlers/DownToHandler.kt @@ -63,7 +63,8 @@ internal class DownToHandler(private val context: CommonBackendContext) : private fun IrExpression.convertToExclusiveLowerBound(progressionType: ProgressionType): IrExpression? { if (progressionType is UnsignedProgressionType) { - if (this.constLongValue == 0L) return null + // On JVM, prefer unsigned counter loop with inclusive bound + if (preferJavaLikeCounterLoop || this.constLongValue == 0L) return null } val irConst = this as? IrConst<*> ?: return null diff --git a/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/loops/handlers/RangeToHandler.kt b/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/loops/handlers/RangeToHandler.kt index 8aabbdc12a0..73ef9c5a636 100644 --- a/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/loops/handlers/RangeToHandler.kt +++ b/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/loops/handlers/RangeToHandler.kt @@ -63,7 +63,8 @@ internal class RangeToHandler(private val context: CommonBackendContext) : private fun IrExpression.convertToExclusiveUpperBound(progressionType: ProgressionType): IrExpression? { if (progressionType is UnsignedProgressionType) { - if (this.constLongValue == -1L) return null + // On JVM, prefer unsigned counter loop with inclusive bound + if (preferJavaLikeCounterLoop || this.constLongValue == -1L) return null } val irConst = this as? IrConst<*> ?: return null diff --git a/compiler/testData/codegen/bytecodeText/forLoop/unsigned/forInConstBoundUnsignedRange.kt b/compiler/testData/codegen/bytecodeText/forLoop/unsigned/forInConstBoundUnsignedRange.kt new file mode 100644 index 00000000000..8fef18311c9 --- /dev/null +++ b/compiler/testData/codegen/bytecodeText/forLoop/unsigned/forInConstBoundUnsignedRange.kt @@ -0,0 +1,76 @@ +// WITH_RUNTIME + +// IMPORTANT! +// Please, when your changes cause failures in bytecodeText tests for 'for' loops, +// examine the resulting bytecode shape carefully. +// Range and progression-based loops generated with Kotlin compiler should be +// as close as possible to Java counter loops ('for (int i = a; i < b; ++i) { ... }'). +// Otherwise it may result in performance regression due to missing HotSpot optimizations. +// Run Kotlin compiler benchmarks (https://github.com/Kotlin/kotlin-benchmarks) +// with compiler built from your changes if you are not sure. + +fun testUIntRangeLiteral(a: UInt): Int { + var s = 0 + for (x in a .. 10u) { + s += x.toInt() + } + return s +} + +fun testULongRangeLiteral(a: ULong): Int { + var s = 0 + for (x in a .. 10u) { + s += x.toInt() + } + return s +} + +fun testUIntUntil(a: UInt): Int { + var s = 0 + for (x in a until 10u) { + s += x.toInt() + } + return s +} + +fun testULongUntil(a: ULong): Int { + var s = 0 + for (x in a until 10u) { + s += x.toInt() + } + return s +} + +fun testUIntDownTo(a: UInt): Int { + var s = 0 + for (x in a downTo 10u) { + s += x.toInt() + } + return s +} + +fun testULongDownTo(a: ULong): Int { + var s = 0 + for (x in a downTo 10u) { + s += x.toInt() + } + return s +} + +// 0 iterator +// 0 hasNext +// 0 next +// 0 getStart +// 0 getEnd +// 0 getFirst +// 0 getLast +// 0 getStep +// 0 INVOKESTATIC kotlin/U(Int|Long).constructor-impl +// 0 INVOKE\w+ kotlin/U(Int|Long).(un)?box-impl + +// JVM_IR_TEMPLATES +// 31 ILOAD +// 21 ISTORE +// 6 IADD +// 0 ISUB +// 3 IINC diff --git a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BytecodeTextTestGenerated.java b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BytecodeTextTestGenerated.java index e9f8690ae0b..254fe628b5c 100644 --- a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BytecodeTextTestGenerated.java +++ b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BytecodeTextTestGenerated.java @@ -2981,6 +2981,12 @@ public class BytecodeTextTestGenerated extends AbstractBytecodeTextTest { KtTestUtil.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("compiler/testData/codegen/bytecodeText/forLoop/unsigned"), Pattern.compile("^(.+)\\.kt$"), null, TargetBackend.JVM, true); } + @Test + @TestMetadata("forInConstBoundUnsignedRange.kt") + public void testForInConstBoundUnsignedRange() throws Exception { + runTest("compiler/testData/codegen/bytecodeText/forLoop/unsigned/forInConstBoundUnsignedRange.kt"); + } + @Test @TestMetadata("forInDownToUIntMinValue.kt") public void testForInDownToUIntMinValue() throws Exception { diff --git a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBytecodeTextTestGenerated.java b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBytecodeTextTestGenerated.java index 7de1643c19f..31ee53a3266 100644 --- a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBytecodeTextTestGenerated.java +++ b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBytecodeTextTestGenerated.java @@ -3089,6 +3089,12 @@ public class IrBytecodeTextTestGenerated extends AbstractIrBytecodeTextTest { KtTestUtil.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("compiler/testData/codegen/bytecodeText/forLoop/unsigned"), Pattern.compile("^(.+)\\.kt$"), null, TargetBackend.JVM_IR, true); } + @Test + @TestMetadata("forInConstBoundUnsignedRange.kt") + public void testForInConstBoundUnsignedRange() throws Exception { + runTest("compiler/testData/codegen/bytecodeText/forLoop/unsigned/forInConstBoundUnsignedRange.kt"); + } + @Test @TestMetadata("forInDownToUIntMinValue.kt") public void testForInDownToUIntMinValue() throws Exception {