From 0d2e19cc4c639c27a93c3ed76d892b16d40dcc9b Mon Sep 17 00:00:00 2001 From: James Iry Date: Fri, 22 Feb 2013 14:01:28 -0800 Subject: SI-7006 Recognize more jump only blocks During ASM code emission we would recognize a block that consisted of ICode-only artifacts (ENTER_SCOPE, EXIT_SCOPE, and LOAD_EXCEPTION) followed by a jump. But we weren't using the same logic to recognize all jump-only blocks. So jump-elision wasn't removing them. And that in fact was why the ASM code emission had to do its special case. This commit makes all jump-only block recognition use the same logic: a jump-only block is one that has 0 or more ICode-only instructions followed by a JUMP. It does't necessarily have to start with a JUMP. There's now a debugWarning if the old NOP emitting code is triggered and test t6102 is enhanced to error if that warning occurs. --- .../scala/tools/nsc/backend/jvm/GenASM.scala | 74 ++++++++++++++-------- test/files/run/t6102.flags | 2 +- 2 files changed, 50 insertions(+), 26 deletions(-) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala b/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala index 6215503c01..c5809cf3f4 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala @@ -2515,21 +2515,13 @@ abstract class GenASM extends SubComponent with BytecodeWriters with GenJVMASM { jcode.emitSWITCH(flatKeys, flatBranches, defaultLabel, MIN_SWITCH_DENSITY) case JUMP(whereto) => - if (nextBlock != whereto) { + if (nextBlock != whereto) jcode goTo labels(whereto) - } else if (m.exh.exists(eh => eh.covers(b))) { // SI-6102: Determine whether eliding this JUMP results in an empty range being covered by some EH. - // If so, emit a NOP in place of the elided JUMP, to avoid "java.lang.ClassFormatError: Illegal exception table range" - val isSthgLeft = b.toList.exists { - case _: LOAD_EXCEPTION => false - case _: SCOPE_ENTER => false - case _: SCOPE_EXIT => false - case _: JUMP => false - case _ => true - } - if (!isSthgLeft) { - emit(asm.Opcodes.NOP) - } + // If so, emit a NOP in place of the elided JUMP, to avoid "java.lang.ClassFormatError: Illegal exception table range" + else if (newNormal.isJumpOnly(b) && m.exh.exists(eh => eh.covers(b))) { + debugwarn("Had a jump only block that wasn't collapsed") + emit(asm.Opcodes.NOP) } case CJUMP(success, failure, cond, kind) => @@ -3084,8 +3076,29 @@ abstract class GenASM extends SubComponent with BytecodeWriters with GenJVMASM { * TODO Eventually, these utilities should be moved to IMethod and reused from normalize() (there's nothing JVM-specific about them). */ object newNormal { - - def startsWithJump(b: BasicBlock): Boolean = { assert(b.nonEmpty, "empty block"); b.firstInstruction.isInstanceOf[JUMP] } + /** + * True if a block is "jump only" which is defined + * as being a block that consists only of 0 or more instructions that + * won't make it to the JVM followed by a JUMP. + */ + def isJumpOnly(b: BasicBlock): Boolean = { + val nonICode = firstNonIcodeOnlyInstructions(b) + // by definition a block has to have a jump, conditional jump, return, or throw + assert(nonICode.hasNext, "empty block") + nonICode.next.isInstanceOf[JUMP] + } + + /** + * Returns the list of instructions in a block that follow all ICode only instructions, + * where an ICode only instruction is one that won't make it to the JVM + */ + private def firstNonIcodeOnlyInstructions(b: BasicBlock): Iterator[Instruction] = { + def isICodeOnlyInstruction(i: Instruction) = i match { + case LOAD_EXCEPTION(_) | SCOPE_ENTER(_) | SCOPE_EXIT(_) => true + case _ => false + } + b.iterator dropWhile isICodeOnlyInstruction + } /** Prune from an exception handler those covered blocks which are jump-only. */ private def coverWhatCountsOnly(m: IMethod): Boolean = { @@ -3093,7 +3106,7 @@ abstract class GenASM extends SubComponent with BytecodeWriters with GenJVMASM { var wasReduced = false for(h <- m.exh) { - val shouldntCover = (h.covered filter startsWithJump) + val shouldntCover = (h.covered filter isJumpOnly) if(shouldntCover.nonEmpty) { wasReduced = true h.covered --= shouldntCover // not removing any block on purpose. @@ -3117,7 +3130,7 @@ abstract class GenASM extends SubComponent with BytecodeWriters with GenJVMASM { def isRedundant(eh: ExceptionHandler): Boolean = { (eh.cls != NoSymbol) && ( // TODO `eh.isFinallyBlock` more readable than `eh.cls != NoSymbol` eh.covered.isEmpty - || (eh.covered forall startsWithJump) + || (eh.covered forall isJumpOnly) ) } @@ -3132,10 +3145,21 @@ abstract class GenASM extends SubComponent with BytecodeWriters with GenJVMASM { wasReduced } - private def isJumpOnly(b: BasicBlock): Option[BasicBlock] = { - b.toList match { - case JUMP(whereto) :: rest => - assert(rest.isEmpty, "A block contains instructions after JUMP (looks like enterIgnoreMode() was itself ignored.)") + /** + * Returns the target of a block that is "jump only" which is defined + * as being a block that consists only of 0 or more instructions that + * won't make it to the JVM followed by a JUMP. + * + * @param b The basic block to examine + * @return Some(target) if b is a "jump only" block or None if it's not + */ + private def getJumpOnlyTarget(b: BasicBlock): Option[BasicBlock] = { + val nonICode = firstNonIcodeOnlyInstructions(b) + // by definition a block has to have a jump, conditional jump, return, or throw + assert(nonICode.nonEmpty, "empty block") + nonICode.next match { + case JUMP(whereto) => + assert(!nonICode.hasNext, "A block contains instructions after JUMP (looks like enterIgnoreMode() was itself ignored.)") Some(whereto) case _ => None } @@ -3167,12 +3191,12 @@ abstract class GenASM extends SubComponent with BytecodeWriters with GenJVMASM { * Precondition: the BasicBlock given as argument starts with an unconditional JUMP. */ private def finalDestination(start: BasicBlock): (BasicBlock, List[BasicBlock]) = { - assert(startsWithJump(start), "not the start of a (single or multi-hop) chain of JUMPs.") + if (settings.debug.value) assert(isJumpOnly(start), "not the start of a (single or multi-hop) chain of JUMPs.") var hops: List[BasicBlock] = Nil var prev = start var done = false do { - done = isJumpOnly(prev) match { + done = getJumpOnlyTarget(prev) match { case Some(dest) => if (dest == start) { return (start, hops) } // leave infinite-loops in place hops ::= prev @@ -3222,7 +3246,7 @@ abstract class GenASM extends SubComponent with BytecodeWriters with GenJVMASM { /* "start" is relative in a cycle, but we call this helper with the "first" entry-point we found. */ def realTarget(jumpStart: BasicBlock): Map[BasicBlock, BasicBlock] = { - assert(startsWithJump(jumpStart), "not part of a jump-chain") + if (settings.debug.value) assert(isJumpOnly(jumpStart), "not part of a jump-chain") val Pair(dest, redundants) = finalDestination(jumpStart) (for(skipOver <- redundants) yield Pair(skipOver, dest)).toMap } @@ -3277,7 +3301,7 @@ abstract class GenASM extends SubComponent with BytecodeWriters with GenJVMASM { while(reachable.nonEmpty) { val h = reachable.head reachable = reachable.tail - if(startsWithJump(h)) { + if(isJumpOnly(h)) { val detour = realTarget(h) if(detour.nonEmpty) { wasReduced = true diff --git a/test/files/run/t6102.flags b/test/files/run/t6102.flags index e35535c8ea..72fe7b1aa0 100644 --- a/test/files/run/t6102.flags +++ b/test/files/run/t6102.flags @@ -1 +1 @@ - -Ydead-code + -Ydead-code -Ydebug -Xfatal-warnings -- cgit v1.2.3