azalea/kotlin-fork
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Do not cache "last" value (i.e., size/length) in lowered for-loop

Browse Source 
iteration over CharSequences.

CharSequences may be mutable (e.g., StringBuilder) and therefore its
contents and length can change within the loop.
This commit is contained in:
Mark Punzalan
2019-09-26 15:25:50 -07:00
committed by max-kammerer
parent 3da3e1cae9
commit c16b59191b
10 changed files with 226 additions and 92 deletions
Download Patch File Download Diff File Expand all files Collapse all files
compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/loops/HeaderInfo.kt
+13 -1
View File
@@ -72,6 +72,7 @@ internal sealed class HeaderInfo(
val first: IrExpression,
val last: IrExpression,
val step: IrExpression,
val canCacheLast: Boolean,
val isReversed: Boolean,
val direction: ProgressionDirection,
val additionalNotEmptyCondition: IrExpression?
@@ -95,7 +96,13 @@ internal class ProgressionHeaderInfo(
direction: ProgressionDirection,
additionalNotEmptyCondition: IrExpression? = null,
val additionalVariables: List<IrVariable> = listOf()
) : HeaderInfo(progressionType, first, last, step, isReversed, direction, additionalNotEmptyCondition) {
) : HeaderInfo(
progressionType, first, last, step,
canCacheLast = true,
isReversed = isReversed,
direction = direction,
additionalNotEmptyCondition = additionalNotEmptyCondition
) {
val canOverflow: Boolean by lazy {
if (canOverflow != null) return@lazy canOverflow
@@ -142,6 +149,7 @@ internal class IndexedGetHeaderInfo(
first: IrExpression,
last: IrExpression,
step: IrExpression,
canCacheLast: Boolean = true,
val objectVariable: IrVariable,
val expressionHandler: IndexedGetIterationHandler
) : HeaderInfo(
@@ -149,6 +157,7 @@ internal class IndexedGetHeaderInfo(
first,
last,
step,
canCacheLast = canCacheLast,
isReversed = false,
direction = ProgressionDirection.INCREASING,
additionalNotEmptyCondition = null
@@ -222,9 +231,12 @@ internal class HeaderInfoBuilder(context: CommonBackendContext, private val scop
private val reversedHandler = ReversedHandler(context, this)
// NOTE: StringIterationHandler MUST come before CharSequenceIterationHandler.
// String is subtype of CharSequence and therefore its handler is more specialized.
private val expressionHandlers = listOf(
ArrayIterationHandler(context),
DefaultProgressionHandler(context),
StringIterationHandler(context),
CharSequenceIterationHandler(context)
)
compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/loops/HeaderProcessor.kt
+101 -85
View File
@@ -43,17 +43,15 @@ internal data class LoopReplacement(
internal sealed class ForLoopHeader(
protected open val headerInfo: HeaderInfo,
val inductionVariable: IrVariable,
val last: IrVariable,
val lastExpression: IrExpression,
val step: IrVariable,
var loopVariable: IrVariable? = null,
val isLastInclusive: Boolean
val isLastInclusive: Boolean,
val declarations: List<IrVariable>
) {
/** Expression used to initialize the loop variable at the beginning of the loop. */
abstract fun initializeLoopVariable(symbols: Symbols<CommonBackendContext>, builder: DeclarationIrBuilder): IrExpression
/** Declarations used in the loop condition and body (e.g., induction variable). */
abstract val declarations: List<IrStatement>
/** Builds a new loop from the old loop. */
abstract fun buildLoop(builder: DeclarationIrBuilder, oldLoop: IrLoop, newBody: IrExpression?): LoopReplacement
@@ -88,14 +86,14 @@ internal sealed class ForLoopHeader(
ProgressionDirection.DECREASING ->
// last <= inductionVar (use `<` if last is exclusive)
irCall(compFun).apply {
putValueArgument(0, irGet(last))
putValueArgument(0, lastExpression)
putValueArgument(1, irGet(inductionVariable))
}
ProgressionDirection.INCREASING ->
// inductionVar <= last (use `<` if last is exclusive)
irCall(compFun).apply {
putValueArgument(0, irGet(inductionVariable))
putValueArgument(1, irGet(last))
putValueArgument(1, lastExpression)
}
ProgressionDirection.UNKNOWN -> {
// If the direction is unknown, we check depending on the "step" value:
@@ -111,7 +109,7 @@ internal sealed class ForLoopHeader(
},
irCall(compFun).apply {
putValueArgument(0, irGet(inductionVariable))
putValueArgument(1, irGet(last))
putValueArgument(1, lastExpression)
}),
context.andand(
irCall(builtIns.lessFunByOperandType[stepKotlinType]!!).apply {
@@ -119,7 +117,7 @@ internal sealed class ForLoopHeader(
putValueArgument(1, if (isLong) irLong(0) else irInt(0))
},
irCall(compFun).apply {
putValueArgument(0, irGet(last))
putValueArgument(0, lastExpression)
putValueArgument(1, irGet(inductionVariable))
})
)
@@ -131,29 +129,22 @@ internal sealed class ForLoopHeader(
internal class ProgressionLoopHeader(
override val headerInfo: ProgressionHeaderInfo,
inductionVariable: IrVariable,
last: IrVariable,
step: IrVariable
) : ForLoopHeader(headerInfo, inductionVariable, last, step, isLastInclusive = true) {
lastExpression: IrExpression,
step: IrVariable,
declarations: List<IrVariable>
) : ForLoopHeader(
headerInfo, inductionVariable,
lastExpression = lastExpression,
step = step,
isLastInclusive = true,
declarations = declarations
) {
override fun initializeLoopVariable(symbols: Symbols<CommonBackendContext>, builder: DeclarationIrBuilder) = with(builder) {
// loopVariable = inductionVariable
irGet(inductionVariable)
}
// For this loop:
//
// for (i in first()..last() step step())
//
// ...the functions may have side-effects so we need to call them in the following order: first() (inductionVariable), last(), step().
// Additional variables come first as they may be needed to the subsequent variables.
//
// In the case of a reversed range, the `inductionVariable` and `last` variables are swapped, therefore the declaration order must be
// swapped to preserve the correct evaluation order.
override val declarations: List<IrStatement>
get() = headerInfo.additionalVariables +
(if (headerInfo.isReversed) listOf(last, inductionVariable) else listOf(inductionVariable, last)) +
step
override fun buildLoop(builder: DeclarationIrBuilder, oldLoop: IrLoop, newBody: IrExpression?) =
with(builder) {
val newLoop = if (headerInfo.canOverflow) {
@@ -169,7 +160,7 @@ internal class ProgressionLoopHeader(
// }
IrDoWhileLoopImpl(oldLoop.startOffset, oldLoop.endOffset, oldLoop.type, oldLoop.origin).apply {
label = oldLoop.label
condition = irNotEquals(irGet(loopVariable!!), irGet(last))
condition = irNotEquals(irGet(loopVariable!!), lastExpression)
body = newBody
}
} else {
@@ -203,9 +194,14 @@ internal class ProgressionLoopHeader(
internal class IndexedGetLoopHeader(
override val headerInfo: IndexedGetHeaderInfo,
inductionVariable: IrVariable,
last: IrVariable,
step: IrVariable
) : ForLoopHeader(headerInfo, inductionVariable, last, step, isLastInclusive = false) {
lastExpression: IrExpression,
step: IrVariable,
declarations: List<IrVariable>
) : ForLoopHeader(
headerInfo, inductionVariable, lastExpression, step,
isLastInclusive = false,
declarations = declarations
) {
override fun initializeLoopVariable(symbols: Symbols<CommonBackendContext>, builder: DeclarationIrBuilder) = with(builder) {
// inductionVar = loopVar[inductionVariable]
@@ -216,16 +212,13 @@ internal class IndexedGetLoopHeader(
}
}
override val declarations: List<IrStatement>
get() = listOf(headerInfo.objectVariable, inductionVariable, last, step)
override fun buildLoop(builder: DeclarationIrBuilder, oldLoop: IrLoop, newBody: IrExpression?): LoopReplacement = with(builder) {
// Loop is lowered into something like:
//
// var inductionVar = 0
// var last = array.size
// var last = objectVariable.size
// while (inductionVar < last) {
// val loopVar = array[inductionVar]
// val loopVar = objectVariable.get(inductionVar)
// inductionVar++
// // Loop body
// }
@@ -278,7 +271,8 @@ internal class HeaderProcessor(
if (it.valueArgumentsCount == 0
&& function.isTopLevel
&& function.getPackageFragment()?.fqName == FqName("kotlin.text")
&& function.name == OperatorNameConventions.ITERATOR) {
&& function.name == OperatorNameConventions.ITERATOR
) {
extensionReceiver
} else {
null
@@ -293,62 +287,84 @@ internal class HeaderProcessor(
?: return null // If the iterable is not supported.
val builder = context.createIrBuilder(scopeOwnerSymbol(), variable.startOffset, variable.endOffset)
with(builder) builder@{
with(headerInfo) {
// For this loop:
//
// for (i in first()..last() step step())
//
// We need to cast first(), last(). and step() to conform to the progression type so
// that operations on the induction variable within the loop are more efficient.
//
// In the above example, if first() is a Long and last() is an Int, this creates a
// LongProgression so last() should be cast to a Long.
val inductionVariable = scope.createTemporaryVariable(
first.castIfNecessary(
progressionType.elementType(context.irBuiltIns),
progressionType.elementCastFunctionName
),
nameHint = "inductionVariable",
isMutable = true
)
with(builder) {
// For this loop:
//
// for (i in first()..last() step step())
//
// We need to cast first(), last(). and step() to conform to the progression type so
// that operations on the induction variable within the loop are more efficient.
//
// In the above example, if first() is a Long and last() is an Int, this creates a
// LongProgression so last() should be cast to a Long.
val inductionVariable = scope.createTemporaryVariable(
headerInfo.first.castIfNecessary(
headerInfo.progressionType.elementType(context.irBuiltIns),
headerInfo.progressionType.elementCastFunctionName
),
nameHint = "inductionVariable",
isMutable = true
)
// Due to features of PSI2IR we can obtain nullable arguments here while actually
// they are non-nullable (the frontend takes care about this). So we need to cast
// them to non-nullable.
// TODO: Confirm if casting to non-nullable is still necessary
val lastValue = scope.createTemporaryVariable(
ensureNotNullable(
last.castIfNecessary(
progressionType.elementType(context.irBuiltIns),
progressionType.elementCastFunctionName
)
),
// Due to features of PSI2IR we can obtain nullable arguments here while actually
// they are non-nullable (the frontend takes care about this). So we need to cast
// them to non-nullable.
// TODO: Confirm if casting to non-nullable is still necessary
val lastExpression = ensureNotNullable(
headerInfo.last.castIfNecessary(
headerInfo.progressionType.elementType(context.irBuiltIns),
headerInfo.progressionType.elementCastFunctionName
)
)
val lastVariableIfCanCacheLast = if (headerInfo.canCacheLast) {
scope.createTemporaryVariable(
lastExpression,
nameHint = "last"
)
} else null
val stepValue = scope.createTemporaryVariable(
ensureNotNullable(
step.castIfNecessary(
progressionType.stepType(context.irBuiltIns),
progressionType.stepCastFunctionName
)
),
nameHint = "step"
)
return when (headerInfo) {
is IndexedGetHeaderInfo -> IndexedGetLoopHeader(
headerInfo,
inductionVariable,
lastValue,
stepValue
val stepVariable = scope.createTemporaryVariable(
ensureNotNullable(
headerInfo.step.castIfNecessary(
headerInfo.progressionType.stepType(context.irBuiltIns),
headerInfo.progressionType.stepCastFunctionName
)
is ProgressionHeaderInfo -> ProgressionLoopHeader(
),
nameHint = "step"
)
return when (headerInfo) {
is IndexedGetHeaderInfo -> IndexedGetLoopHeader(
headerInfo,
inductionVariable,
if (headerInfo.canCacheLast) irGet(lastVariableIfCanCacheLast!!) else lastExpression,
stepVariable,
listOfNotNull(headerInfo.objectVariable, inductionVariable, lastVariableIfCanCacheLast, stepVariable)
)
is ProgressionHeaderInfo -> {
// For this loop:
//
// for (i in first()..last() step step())
//
// ...the functions may have side-effects so we need to call them in the following order: first() (inductionVariable), last(), step().
// Additional variables come first as they may be needed to the subsequent variables.
//
// In the case of a reversed range, the `inductionVariable` and `last` variables are swapped, therefore the declaration order must be
// swapped to preserve the correct evaluation order.
val declarations = headerInfo.additionalVariables + (
if (headerInfo.isReversed)
listOfNotNull(lastVariableIfCanCacheLast, inductionVariable)
else
listOfNotNull(inductionVariable, lastVariableIfCanCacheLast)
) +
stepVariable
ProgressionLoopHeader(
headerInfo,
inductionVariable,
lastValue,
stepValue
if (headerInfo.canCacheLast) irGet(lastVariableIfCanCacheLast!!) else lastExpression,
stepVariable,
declarations
)
}
}
compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/loops/ProgressionHandlers.kt
+27 -6
View File
@@ -330,7 +330,11 @@ internal class DefaultProgressionHandler(private val context: CommonBackendConte
}
}
internal abstract class IndexedGetIterationHandler(protected val context: CommonBackendContext) : ExpressionHandler {
internal abstract class IndexedGetIterationHandler(
protected val context: CommonBackendContext,
val canCacheLast: Boolean = true
) :
ExpressionHandler {
override fun build(expression: IrExpression, scopeOwner: IrSymbol): HeaderInfo? =
with(context.createIrBuilder(scopeOwner, expression.startOffset, expression.endOffset)) {
// Consider the case like:
@@ -348,19 +352,20 @@ internal abstract class IndexedGetIterationHandler(protected val context: Common
//
// This also ensures that the semantics of re-assignment of array variables used in the loop is consistent with the semantics
// proposed in https://youtrack.jetbrains.com/issue/KT-21354.
val arrayReference = scope.createTemporaryVariable(
val objectVariable = scope.createTemporaryVariable(
expression, nameHint = "indexedObject"
)
val last = irCall(expression.type.sizePropertyGetter).apply {
dispatchReceiver = irGet(arrayReference)
dispatchReceiver = irGet(objectVariable)
}
IndexedGetHeaderInfo(
first = irInt(0),
last = last,
step = irInt(1),
objectVariable = arrayReference,
canCacheLast = canCacheLast,
objectVariable = objectVariable,
expressionHandler = this@IndexedGetIterationHandler
)
}
@@ -381,8 +386,24 @@ internal class ArrayIterationHandler(context: CommonBackendContext) : IndexedGet
get() = getClass()!!.functions.first { it.name.asString() == "get" }
}
/** Builds a [HeaderInfo] for iteration over characters in a `CharacterSequence`. */
internal class CharSequenceIterationHandler(context: CommonBackendContext) : IndexedGetIterationHandler(context) {
/** Builds a [HeaderInfo] for iteration over characters in a [String]. */
internal class StringIterationHandler(context: CommonBackendContext) : IndexedGetIterationHandler(context) {
override fun match(expression: IrExpression) = expression.type.isString()
override val IrType.sizePropertyGetter
get() = getClass()!!.properties.first { it.name.asString() == "length" }.getter!!
override val IrType.getFunction
get() = getClass()!!.functions.first { it.name.asString() == "get" }
}
/**
* Builds a [HeaderInfo] for iteration over characters in a [CharSequence].
*
* Note: The value for "last" can NOT be cached (i.e., stored in a variable) because the size/length can change within the loop. This means
* that "last" is re-evaluated with each iteration of the loop.
*/
internal class CharSequenceIterationHandler(context: CommonBackendContext) : IndexedGetIterationHandler(context, canCacheLast = false) {
override fun match(expression: IrExpression) = expression.type.isSubtypeOfClass(context.ir.symbols.charSequence)
// The lowering operates on subtypes of CharSequence. Therefore, the IrType could be
compiler/testData/codegen/box/ranges/forInCharSequenceLengthDecreasedInLoopBody.kt
Vendored
+16
View File
@@ -0,0 +1,16 @@
// KJS_WITH_FULL_RUNTIME
// WITH_RUNTIME
import kotlin.test.*
fun box(): String {
val sb = StringBuilder("1234")
val result = StringBuilder()
for (c in sb) {
sb.clear()
result.append(c)
}
assertEquals("", sb.toString())
assertEquals("1", result.toString())
return "OK"
}
compiler/testData/codegen/box/ranges/forInCharSequenceLengthIncreasedInLoopBody.kt
Vendored
+19
View File
@@ -0,0 +1,19 @@
// KJS_WITH_FULL_RUNTIME
// WITH_RUNTIME
import kotlin.test.*
fun box(): String {
val sb = StringBuilder("1234")
val result = StringBuilder()
var ctr = 0
for (c in sb) {
if (ctr % 2 == 0)
sb.append('x')
ctr++
result.append(c)
}
assertEquals("1234xxxx", sb.toString())
assertEquals("1234xxxx", result.toString())
return "OK"
}
compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java
Generated
+10
View File
@@ -18819,6 +18819,16 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest {
runTest("compiler/testData/codegen/box/ranges/forByteProgressionWithIntIncrement.kt");
}
@TestMetadata("forInCharSequenceLengthDecreasedInLoopBody.kt")
public void testForInCharSequenceLengthDecreasedInLoopBody() throws Exception {
runTest("compiler/testData/codegen/box/ranges/forInCharSequenceLengthDecreasedInLoopBody.kt");
}
@TestMetadata("forInCharSequenceLengthIncreasedInLoopBody.kt")
public void testForInCharSequenceLengthIncreasedInLoopBody() throws Exception {
runTest("compiler/testData/codegen/box/ranges/forInCharSequenceLengthIncreasedInLoopBody.kt");
}
@TestMetadata("forInRangeLiteralWithMixedTypeBounds.kt")
public void testForInRangeLiteralWithMixedTypeBounds() throws Exception {
runTest("compiler/testData/codegen/box/ranges/forInRangeLiteralWithMixedTypeBounds.kt");
compiler/tests/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java
Generated
+10
View File
@@ -18819,6 +18819,16 @@ public class LightAnalysisModeTestGenerated extends AbstractLightAnalysisModeTes
runTest("compiler/testData/codegen/box/ranges/forByteProgressionWithIntIncrement.kt");
}
@TestMetadata("forInCharSequenceLengthDecreasedInLoopBody.kt")
public void testForInCharSequenceLengthDecreasedInLoopBody() throws Exception {
runTest("compiler/testData/codegen/box/ranges/forInCharSequenceLengthDecreasedInLoopBody.kt");
}
@TestMetadata("forInCharSequenceLengthIncreasedInLoopBody.kt")
public void testForInCharSequenceLengthIncreasedInLoopBody() throws Exception {
runTest("compiler/testData/codegen/box/ranges/forInCharSequenceLengthIncreasedInLoopBody.kt");
}
@TestMetadata("forInRangeLiteralWithMixedTypeBounds.kt")
public void testForInRangeLiteralWithMixedTypeBounds() throws Exception {
runTest("compiler/testData/codegen/box/ranges/forInRangeLiteralWithMixedTypeBounds.kt");
compiler/tests/org/jetbrains/kotlin/codegen/ir/IrBlackBoxCodegenTestGenerated.java
Generated
+10
View File
@@ -17704,6 +17704,16 @@ public class IrBlackBoxCodegenTestGenerated extends AbstractIrBlackBoxCodegenTes
runTest("compiler/testData/codegen/box/ranges/forByteProgressionWithIntIncrement.kt");
}
@TestMetadata("forInCharSequenceLengthDecreasedInLoopBody.kt")
public void testForInCharSequenceLengthDecreasedInLoopBody() throws Exception {
runTest("compiler/testData/codegen/box/ranges/forInCharSequenceLengthDecreasedInLoopBody.kt");
}
@TestMetadata("forInCharSequenceLengthIncreasedInLoopBody.kt")
public void testForInCharSequenceLengthIncreasedInLoopBody() throws Exception {
runTest("compiler/testData/codegen/box/ranges/forInCharSequenceLengthIncreasedInLoopBody.kt");
}
@TestMetadata("forInRangeLiteralWithMixedTypeBounds.kt")
public void testForInRangeLiteralWithMixedTypeBounds() throws Exception {
runTest("compiler/testData/codegen/box/ranges/forInRangeLiteralWithMixedTypeBounds.kt");
js/js.tests/test/org/jetbrains/kotlin/js/test/ir/semantics/IrJsCodegenBoxTestGenerated.java
Generated
+10
View File
@@ -14639,6 +14639,16 @@ public class IrJsCodegenBoxTestGenerated extends AbstractIrJsCodegenBoxTest {
runTest("compiler/testData/codegen/box/ranges/forByteProgressionWithIntIncrement.kt");
}
@TestMetadata("forInCharSequenceLengthDecreasedInLoopBody.kt")
public void testForInCharSequenceLengthDecreasedInLoopBody() throws Exception {
runTest("compiler/testData/codegen/box/ranges/forInCharSequenceLengthDecreasedInLoopBody.kt");
}
@TestMetadata("forInCharSequenceLengthIncreasedInLoopBody.kt")
public void testForInCharSequenceLengthIncreasedInLoopBody() throws Exception {
runTest("compiler/testData/codegen/box/ranges/forInCharSequenceLengthIncreasedInLoopBody.kt");
}
@TestMetadata("forInRangeLiteralWithMixedTypeBounds.kt")
public void testForInRangeLiteralWithMixedTypeBounds() throws Exception {
runTest("compiler/testData/codegen/box/ranges/forInRangeLiteralWithMixedTypeBounds.kt");
js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/JsCodegenBoxTestGenerated.java
Generated
+10
View File
@@ -15794,6 +15794,16 @@ public class JsCodegenBoxTestGenerated extends AbstractJsCodegenBoxTest {
runTest("compiler/testData/codegen/box/ranges/forByteProgressionWithIntIncrement.kt");
}
@TestMetadata("forInCharSequenceLengthDecreasedInLoopBody.kt")
public void testForInCharSequenceLengthDecreasedInLoopBody() throws Exception {
runTest("compiler/testData/codegen/box/ranges/forInCharSequenceLengthDecreasedInLoopBody.kt");
}
@TestMetadata("forInCharSequenceLengthIncreasedInLoopBody.kt")
public void testForInCharSequenceLengthIncreasedInLoopBody() throws Exception {
runTest("compiler/testData/codegen/box/ranges/forInCharSequenceLengthIncreasedInLoopBody.kt");
}
@TestMetadata("forInRangeLiteralWithMixedTypeBounds.kt")
public void testForInRangeLiteralWithMixedTypeBounds() throws Exception {
runTest("compiler/testData/codegen/box/ranges/forInRangeLiteralWithMixedTypeBounds.kt");