daniellansun commented on code in PR #2707:
URL: https://github.com/apache/groovy/pull/2707#discussion_r3604149086
##########
src/test/groovy/org/codehaus/groovy/classgen/asm/PeepholeOptimizingMethodVisitorTest.groovy:
##########
@@ -366,6 +441,788 @@ final class PeepholeOptimizingMethodVisitorTest {
]
}
+ @Test
+ void preservesStandaloneCheckcastBeforeReturn() {
+ def bytecode = sequenceFor('(Ljava/lang/Object;)V') {
+ visitVarInsn(Opcodes.ALOAD, 0)
+ visitInsn(Opcodes.NOP)
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/String')
+ visitInsn(Opcodes.POP)
+ visitInsn(RETURN)
+ }
+
+ // Standalone CHECKCAST before POP is dropped; before RETURN it is
flushed
+ // and the value is popped so the void return remains verifiable.
+ def returnOnly = sequenceFor('(Ljava/lang/Object;)V') {
+ visitVarInsn(Opcodes.ALOAD, 0)
+ visitInsn(Opcodes.NOP)
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/String')
+ visitInsn(RETURN)
+ }
+
+ assert opcodeLines(bytecode) == ['ALOAD 0', 'NOP', 'POP', 'RETURN']
+ assert opcodeLines(returnOnly) == ['ALOAD 0', 'NOP', 'CHECKCAST
java/lang/String', 'POP', 'RETURN']
+ }
+
+ @Test
+ void preservesAttachedCheckcastSideEffectBeforeReturn() {
+ def bytecode = sequenceFor('(Ljava/lang/Object;)V') {
+ visitVarInsn(Opcodes.ALOAD, 0)
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/String')
+ visitInsn(RETURN)
+ }
+
+ // Cast is kept for its ClassCastException side effect; POP keeps void
return valid.
+ assert opcodeLines(bytecode) == [
+ 'ALOAD 0',
+ 'CHECKCAST java/lang/String',
+ 'POP',
+ 'RETURN',
+ ]
+ }
+
+ @Test
+ void wrapIsIdempotentAndPropagatesNull() {
+ assert PeepholeOptimizingMethodVisitor.wrap(null) == null
+
+ def methodNode = new MethodNode(CompilerConfiguration.ASM_API_VERSION,
ACC_PUBLIC | ACC_STATIC, 'sample', '()V', null, null)
+ def once = PeepholeOptimizingMethodVisitor.wrap(methodNode)
+ assert once instanceof PeepholeOptimizingMethodVisitor
+ assert PeepholeOptimizingMethodVisitor.wrap(once).is(once)
+ }
+
+ @Test
+ void printTraceBytecodeFindsNestedTracer() {
+ def textifier = new Textifier()
+ def tracer = new TraceMethodVisitor(textifier)
+ def peephole = new PeepholeOptimizingMethodVisitor(tracer)
+ peephole.visitCode()
+ peephole.visitInsn(Opcodes.ICONST_1)
+ peephole.visitVarInsn(Opcodes.ISTORE, 0) // keep the constant live for
tracing
+ peephole.visitInsn(RETURN)
+ peephole.visitMaxs(0, 0)
+ peephole.visitEnd()
+
+ def out = new StringWriter()
+ def printer = new PrintWriter(out)
+ assert PeepholeOptimizingMethodVisitor.printTraceBytecode(peephole,
printer)
+ printer.flush()
+ assert out.toString().contains('ICONST_1')
+ assert out.toString().contains('ISTORE')
+ assert out.toString().contains('RETURN')
+
+ assert !PeepholeOptimizingMethodVisitor.printTraceBytecode(null,
printer)
+ assert !PeepholeOptimizingMethodVisitor.printTraceBytecode(new
MethodNode(CompilerConfiguration.ASM_API_VERSION, ACC_PUBLIC | ACC_STATIC, 'x',
'()V', null, null), printer)
+ }
+
+ @Test
+ void classVisitorWrapsEveryMethodAndSkipsNullDelegates() {
+ def written = []
+ def delegate = new
org.objectweb.asm.ClassVisitor(CompilerConfiguration.ASM_API_VERSION) {
+ @Override
+ MethodVisitor visitMethod(int access, String name, String
descriptor, String signature, String[] exceptions) {
+ if (name == 'skip') {
+ return null
+ }
+ def mn = new MethodNode(CompilerConfiguration.ASM_API_VERSION,
access, name, descriptor, signature, exceptions)
+ written << mn
+ return mn
+ }
+ }
+ def peepholeClass = new PeepholeOptimizingClassVisitor(delegate)
+
+ def optimized = peepholeClass.visitMethod(ACC_PUBLIC | ACC_STATIC,
'run', '()V', null, null)
+ assert optimized instanceof PeepholeOptimizingMethodVisitor
+ optimized.visitCode()
+ optimized.visitLdcInsn(0)
+ optimized.visitVarInsn(Opcodes.ISTORE, 0)
+ optimized.visitInsn(RETURN)
+ optimized.visitMaxs(0, 0)
+ optimized.visitEnd()
+
+ assert peepholeClass.visitMethod(ACC_PUBLIC | ACC_STATIC, 'skip',
'()V', null, null) == null
+ assert written.size() == 1
+ assert opcodeLines(traceSequence(written[0])) == ['ICONST_0', 'ISTORE
0', 'RETURN']
+ }
+
+ @Test
+ void doesNotRewriteNonMatchingCompareJumps() {
+ def zeroLabel = new Label()
+ def zeroWithIfnull = sequenceFor('(I)V') {
+ visitVarInsn(Opcodes.ILOAD, 0)
+ visitLdcInsn(0)
+ visitJumpInsn(Opcodes.IFNULL, zeroLabel)
+ visitInsn(RETURN)
+ visitLabel(zeroLabel)
+ visitInsn(RETURN)
+ }
+ assert opcodeLines(zeroWithIfnull)[1] == 'ICONST_0'
+ assert opcodeLines(zeroWithIfnull)[2].startsWith('IFNULL')
+
+ def nullLabel = new Label()
+ def nullWithIfeq = sequenceFor('(Ljava/lang/Object;)V') {
+ visitVarInsn(Opcodes.ALOAD, 0)
+ visitInsn(Opcodes.ACONST_NULL)
+ visitJumpInsn(Opcodes.IFEQ, nullLabel)
+ visitInsn(RETURN)
+ visitLabel(nullLabel)
+ visitInsn(RETURN)
+ }
+ assert opcodeLines(nullWithIfeq)[1] == 'ACONST_NULL'
+ assert opcodeLines(nullWithIfeq)[2].startsWith('IFEQ')
+ }
+
+ @Test
+ void preservesMismatchedPopSizes() {
+ def bytecode = sequenceFor('(IJ)V') {
+ visitVarInsn(Opcodes.ILOAD, 0)
+ visitInsn(Opcodes.POP2)
+ visitVarInsn(Opcodes.LLOAD, 1)
+ visitInsn(Opcodes.POP)
+ visitInsn(RETURN)
+ }
+
+ assert opcodeLines(bytecode) == [
+ 'ILOAD 0',
+ 'POP2',
+ 'LLOAD 1',
+ 'POP',
+ 'RETURN',
+ ]
+ }
+
+ @Test
+ void attachesCheckcastToReferenceConstantsAndDropsDeadCastChains() {
+ def type = org.objectweb.asm.Type.getType(String)
+ def handle = new Handle(Opcodes.H_INVOKESTATIC, 'Owner', 'boot',
'()V', false)
+ def bytecode = sequenceFor {
+ visitLdcInsn('text')
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/String')
+ visitInsn(Opcodes.POP)
+ visitLdcInsn(type)
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/Class')
+ visitInsn(Opcodes.POP)
+ visitLdcInsn(handle)
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/invoke/MethodHandle')
+ visitInsn(Opcodes.POP)
+ visitInsn(Opcodes.ACONST_NULL)
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/String')
+ visitInsn(Opcodes.POP)
+ visitInsn(RETURN)
+ }
+
+ assert opcodeLines(bytecode) == ['RETURN']
+ }
+
+ @Test
+ void flushesBeforeMethodAndInvokeDynamicCalls() {
+ def handle = new Handle(Opcodes.H_INVOKESTATIC, 'Owner', 'bootstrap',
'()Ljava/lang/invoke/CallSite;', false)
+ def bytecode = sequenceFor('(Ljava/lang/Object;)V') {
+ visitVarInsn(Opcodes.ALOAD, 0)
+ visitMethodInsn(Opcodes.INVOKEVIRTUAL, 'java/lang/Object',
'toString', '()Ljava/lang/String;', false)
+ visitInsn(Opcodes.POP)
+ visitLdcInsn(1)
+ visitInvokeDynamicInsn('dyn', '()I', handle)
+ visitInsn(Opcodes.POP)
+ visitInsn(RETURN)
+ }
+
+ def lines = opcodeLines(bytecode)
+ assert lines[0] == 'ALOAD 0'
+ assert lines[1] == 'INVOKEVIRTUAL java/lang/Object.toString
()Ljava/lang/String;'
+ assert lines[2] == 'POP'
+ assert lines[3] == 'ICONST_1'
+ assert lines[4].startsWith('INVOKEDYNAMIC dyn')
+ assert lines[-2] == 'POP'
+ assert lines[-1] == 'RETURN'
+ }
+
+ @Test
+ void flushesPendingLoadOnLineNumberAndNonCheckcastTypeInsn() {
+ def start = new Label()
+ def bytecode = sequenceFor('(Ljava/lang/Object;)V') {
+ visitLabel(start)
+ visitVarInsn(Opcodes.ALOAD, 0)
+ visitLineNumber(42, start)
+ visitTypeInsn(Opcodes.INSTANCEOF, 'java/lang/String')
+ visitInsn(Opcodes.POP)
+ visitInsn(RETURN)
+ }
+
+ assert opcodeLines(bytecode) == [
+ 'ALOAD 0',
+ 'INSTANCEOF java/lang/String',
+ 'POP',
+ 'RETURN',
+ ]
+ }
+
+ @Test
+ void doesNotAttachSecondCheckcastAndFlushesIincOnConflict() {
+ def chainedCasts =
sequenceFor('(Ljava/lang/Object;)Ljava/lang/Object;') {
+ visitVarInsn(Opcodes.ALOAD, 0)
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/CharSequence')
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/String')
+ visitInsn(Opcodes.ARETURN)
+ }
+ assert opcodeLines(chainedCasts) == [
+ 'ALOAD 0',
+ 'CHECKCAST java/lang/CharSequence',
+ 'CHECKCAST java/lang/String',
+ 'ARETURN',
+ ]
+
+ def secondIincFlushes = sequenceFor('(I)V') {
+ visitVarInsn(Opcodes.ILOAD, 0)
+ visitIincInsn(0, 1)
+ visitIincInsn(0, 2) // already has IINC → flush load+first IINC,
emit second
+ visitInsn(Opcodes.POP)
+ visitInsn(RETURN)
+ }
+ assert opcodeLines(secondIincFlushes) == [
+ 'ILOAD 0',
+ 'IINC 0 1',
+ 'IINC 0 2',
+ 'POP',
+ 'RETURN',
+ ]
Review Comment:
Yes — if slot 0 were known to be a compiler temp (no LVT entry), ILOAD+POP
after
the IINCs could go away. That needs LVT / “is this a temp?” info the current
single-window visitor doesn’t have. Follow-up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
