JVM: do not remove NOPs in PopBackwardPropagationTransformer

There is a pass that removes NOPs and runs afterwards anyway.

compiler/backend/src/org/jetbrains/kotlin/codegen/optimization/boxing/PopBackwardPropagationTransformer.kt

@@ -19,10 +19,8 @@ package org.jetbrains.kotlin.codegen.optimization.boxing
 import org.jetbrains.kotlin.codegen.optimization.OptimizationMethodVisitor
 import org.jetbrains.kotlin.codegen.optimization.common.debugText
 import org.jetbrains.kotlin.codegen.optimization.common.isLoadOperation
-import org.jetbrains.kotlin.codegen.optimization.common.isMeaningful
 import org.jetbrains.kotlin.codegen.optimization.fixStack.peekWords
 import org.jetbrains.kotlin.codegen.optimization.fixStack.top
-import org.jetbrains.kotlin.codegen.optimization.removeNodeGetNext
 import org.jetbrains.kotlin.codegen.optimization.transformer.MethodTransformer
 import org.jetbrains.org.objectweb.asm.Opcodes
 import org.jetbrains.org.objectweb.asm.tree.*
@@ -32,59 +30,49 @@ import org.jetbrains.org.objectweb.asm.tree.analysis.SourceInterpreter
 import org.jetbrains.org.objectweb.asm.tree.analysis.SourceValue
 import java.util.*
 
+private typealias Transformation = (AbstractInsnNode) -> Unit
+
 class PopBackwardPropagationTransformer : MethodTransformer() {
     override fun transform(internalClassName: String, methodNode: MethodNode) {
         if (!OptimizationMethodVisitor.canBeOptimizedUsingSourceInterpreter(methodNode)) return
-        Transformer(methodNode).transform()
+        if (methodNode.instructions.any { it.isPop() || it.isPurePush() }) {
+            Transformer(methodNode).transform()
+        }
     }
 
     private class Transformer(val methodNode: MethodNode) {
-        private interface Transformation {
+        private val REPLACE_WITH_NOP: Transformation = { insnList.set(it, InsnNode(Opcodes.NOP)) }
-            fun apply(insn: AbstractInsnNode)
+        private val REPLACE_WITH_POP1: Transformation = { insnList.set(it, InsnNode(Opcodes.POP)) }
-        }
+        private val REPLACE_WITH_POP2: Transformation = { insnList.set(it, InsnNode(Opcodes.POP2)) }
-
+        private val INSERT_POP1_AFTER: Transformation = { insnList.insert(it, InsnNode(Opcodes.POP)) }
-        private inline fun Transformation(crossinline body: (AbstractInsnNode) -> Unit): Transformation =
+        private val INSERT_POP2_AFTER: Transformation = { insnList.insert(it, InsnNode(Opcodes.POP2)) }
-            object : Transformation {
-                override fun apply(insn: AbstractInsnNode) {
-                    body(insn)
-                }
-            }
-
-        private val REPLACE_WITH_NOP = Transformation { insnList.set(it, createRemovableNopInsn()) }
-        private val REPLACE_WITH_POP1 = Transformation { insnList.set(it, InsnNode(Opcodes.POP)) }
-        private val REPLACE_WITH_POP2 = Transformation { insnList.set(it, InsnNode(Opcodes.POP2)) }
-        private val INSERT_POP1_AFTER = Transformation { insnList.insert(it, InsnNode(Opcodes.POP)) }
-        private val INSERT_POP2_AFTER = Transformation { insnList.insert(it, InsnNode(Opcodes.POP2)) }
 
         private val insnList = methodNode.instructions
-
         private val insns = insnList.toArray()
 
         private val dontTouchInsnIndices = BitSet(insns.size)
-        private val transformations = hashMapOf<AbstractInsnNode, Transformation>()
-        private val removableNops = hashSetOf<InsnNode>()
 
-        private val frames by lazy { analyzeMethodBody() }
+        private val transformations = hashMapOf<AbstractInsnNode, Transformation>()
+        private val frames = analyzeMethodBody()
 
         fun transform() {
-            if (insns.none { it.isPop() || it.isPurePush() }) return
+            for ((i, insn) in insns.withIndex()) {
-
+                if (insn.opcode == Opcodes.POP && frames[i] != null) {
-            computeTransformations()
+                    val inputTop = getInputTop(insn)
-            for ((insn, transformation) in transformations.entries) {
+                    val sources = inputTop.insns
-                transformation.apply(insn)
+                    if (sources.all { !isDontTouch(it) } && sources.any { isTransformablePopOperand(it) }) {
+                        transformations[insn] = REPLACE_WITH_NOP
+                        sources.forEach { propagatePopBackwards(it, inputTop.size) }
+                    }
+                }
+            }
+            for ((insn, transformation) in transformations.entries) {
+                transformation(insn)
             }
-            postprocessNops()
         }
 
         private fun analyzeMethodBody(): Array<out Frame<SourceValue>?> {
-            val frames = Analyzer<SourceValue>(HazardsTrackingInterpreter()).analyze("fake", methodNode)
+            val frames = Analyzer(HazardsTrackingInterpreter()).analyze("fake", methodNode)
-
-            postprocessStackHazards(frames)
-
-            return frames
-        }
-
-        private fun postprocessStackHazards(frames: Array<out Frame<SourceValue>?>) {
             val insns = methodNode.instructions.toArray()
             for (i in frames.indices) {
                 val frame = frames[i] ?: continue
@@ -113,6 +101,7 @@ class PopBackwardPropagationTransformer : MethodTransformer() {
                     }
                 }
             }
+            return frames
         }
 
         private fun throwIncorrectBytecode(insn: AbstractInsnNode?, frame: Frame<SourceValue>): Nothing {
@@ -164,39 +153,16 @@ class PopBackwardPropagationTransformer : MethodTransformer() {
             }
         }
 
-
-        private fun computeTransformations() {
-            transformations.clear()
-
-            for (i in insns.indices) {
-                if (frames[i] == null) continue
-                val insn = insns[i]
-
-                if (insn.opcode == Opcodes.POP) {
-                    propagatePopBackwards(insn, 0)
-                }
-            }
-        }
-
         private fun propagatePopBackwards(insn: AbstractInsnNode, poppedValueSize: Int) {
             if (transformations.containsKey(insn)) return
 
             when {
-                insn.opcode == Opcodes.POP -> {
-                    val inputTop = getInputTop(insn)
-                    val sources = inputTop.insns
-                    if (sources.all { !isDontTouch(it) } && sources.any { isTransformablePopOperand(it) }) {
-                        transformations[insn] = replaceWithNopTransformation()
-                        sources.forEach { propagatePopBackwards(it, inputTop.size) }
-                    }
-                }
-
                 insn.opcode == Opcodes.CHECKCAST -> {
                     val inputTop = getInputTop(insn)
                     val sources = inputTop.insns
                     val resultType = (insn as TypeInsnNode).desc
                     if (sources.all { !isDontTouch(it) } && sources.any { isTransformableCheckcastOperand(it, resultType) }) {
-                        transformations[insn] = replaceWithNopTransformation()
+                        transformations[insn] = REPLACE_WITH_NOP
                         sources.forEach { propagatePopBackwards(it, inputTop.size) }
                     } else {
                         transformations[insn] = insertPopAfterTransformation(poppedValueSize)
@@ -204,22 +170,21 @@ class PopBackwardPropagationTransformer : MethodTransformer() {
                 }
 
                 insn.isPrimitiveBoxing() -> {
-                    val boxedValueSize = getInputTop(insn).size
+                    transformations[insn] = replaceWithPopTransformation(getInputTop(insn).size)
-                    transformations[insn] = replaceWithPopTransformation(boxedValueSize)
                 }
 
                 insn.isPurePush() -> {
-                    transformations[insn] = replaceWithNopTransformation()
+                    transformations[insn] = REPLACE_WITH_NOP
                 }
 
                 insn.isPrimitiveTypeConversion() -> {
                     val inputTop = getInputTop(insn)
                     val sources = inputTop.insns
                     if (sources.all { !isDontTouch(it) }) {
-                        transformations[insn] = replaceWithNopTransformation()
+                        transformations[insn] = REPLACE_WITH_NOP
                         sources.forEach { propagatePopBackwards(it, inputTop.size) }
                     } else {
-                        transformations[insn] = replaceWithPopTransformation(poppedValueSize)
+                        transformations[insn] = replaceWithPopTransformation(inputTop.size)
                     }
                 }
 
@@ -229,39 +194,6 @@ class PopBackwardPropagationTransformer : MethodTransformer() {
             }
         }
 
-        private fun postprocessNops() {
-            var node: AbstractInsnNode? = insnList.first
-            var hasRemovableNops = false
-            while (node != null) {
-                node = node.next
-                val begin = node ?: break
-                while (node != null && node !is LabelNode) {
-                    if (node in removableNops) {
-                        hasRemovableNops = true
-                    }
-                    node = node.next
-                }
-                val end = node
-                if (hasRemovableNops) {
-                    removeUnneededNopsInRange(begin, end)
-                }
-                hasRemovableNops = false
-            }
-        }
-
-        private fun removeUnneededNopsInRange(begin: AbstractInsnNode, end: AbstractInsnNode?) {
-            var node: AbstractInsnNode? = begin
-            var keepNop = true
-            while (node != null && node != end) {
-                if (node in removableNops && !keepNop) {
-                    node = insnList.removeNodeGetNext(node)
-                } else {
-                    if (node.isMeaningful) keepNop = false
-                    node = node.next
-                }
-            }
-        }
-
         private fun replaceWithPopTransformation(size: Int): Transformation =
             when (size) {
                 1 -> REPLACE_WITH_POP1
@@ -276,12 +208,6 @@ class PopBackwardPropagationTransformer : MethodTransformer() {
                 else -> throw AssertionError("Unexpected pop value size: $size")
             }
 
-        private fun replaceWithNopTransformation(): Transformation =
-            REPLACE_WITH_NOP
-
-        private fun createRemovableNopInsn() =
-            InsnNode(Opcodes.NOP).apply { removableNops.add(this) }
-
         private fun getInputTop(insn: AbstractInsnNode): SourceValue {
             val i = insnList.indexOf(insn)
             val frame = frames[i] ?: throw AssertionError("Unexpected dead instruction #$i")
@@ -301,16 +227,13 @@ class PopBackwardPropagationTransformer : MethodTransformer() {
 }
 
 fun AbstractInsnNode.isPurePush() =
-    isLoadOperation() ||
+    isLoadOperation() || opcode in Opcodes.ACONST_NULL..Opcodes.LDC + 2 || isUnitInstance()
-            opcode in Opcodes.ACONST_NULL..Opcodes.LDC + 2 ||
-            isUnitInstance()
 
 fun AbstractInsnNode.isPop() =
     opcode == Opcodes.POP || opcode == Opcodes.POP2
 
 fun AbstractInsnNode.isUnitInstance() =
-    opcode == Opcodes.GETSTATIC &&
+    opcode == Opcodes.GETSTATIC && this is FieldInsnNode && owner == "kotlin/Unit" && name == "INSTANCE"
-            this is FieldInsnNode && owner == "kotlin/Unit" && name == "INSTANCE"
 
 fun AbstractInsnNode.isPrimitiveTypeConversion() =
     opcode in Opcodes.I2L..Opcodes.I2S