[
https://issues.apache.org/jira/browse/GROOVY-12162?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18097108#comment-18097108
]
ASF GitHub Bot commented on GROOVY-12162:
-----------------------------------------
daniellansun commented on code in PR #2707:
URL: https://github.com/apache/groovy/pull/2707#discussion_r3604084417
##########
src/test/groovy/org/codehaus/groovy/classgen/asm/PeepholeOptimizingMethodVisitorTest.groovy:
##########
@@ -329,16 +329,91 @@ final class PeepholeOptimizingMethodVisitorTest {
assert opcodeLines(bytecode) == ['IINC 0 1', 'RETURN']
}
+ @Test
+ void removesLoadAndAttachedCheckcastBeforePop() {
+ def bytecode = sequenceFor('(Ljava/lang/Object;)V') {
+ visitVarInsn(Opcodes.ALOAD, 0)
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/String')
+ visitInsn(Opcodes.POP)
+ visitInsn(RETURN)
+ }
+
+ // ALOAD + CHECKCAST are both dead once the value is discarded.
+ assert opcodeLines(bytecode) == ['RETURN']
+ }
+
@Test
void removesStandaloneCheckcastBeforePop() {
def bytecode = sequenceFor('(Ljava/lang/Object;)V') {
visitVarInsn(Opcodes.ALOAD, 0)
+ visitInsn(Opcodes.NOP) // force the load to flush so CHECKCAST is
standalone
visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/String')
visitInsn(Opcodes.POP)
visitInsn(RETURN)
}
- assert opcodeLines(bytecode) == ['ALOAD 0', 'POP', 'RETURN']
+ assert opcodeLines(bytecode) == ['ALOAD 0', 'NOP', 'POP', 'RETURN']
+ }
+
+ @Test
+ void rewritesNullComparisonsAgainstAconstNull() {
+ def nullComparisons = [
+ (Opcodes.IF_ACMPEQ): 'IFNULL',
+ (Opcodes.IF_ACMPNE): 'IFNONNULL',
+ ]
+
+ nullComparisons.each { opcode, expected ->
+ def label = new Label()
+ def bytecode = sequenceFor('(Ljava/lang/Object;)V') {
+ visitVarInsn(Opcodes.ALOAD, 0)
+ visitInsn(Opcodes.ACONST_NULL)
+ visitJumpInsn(opcode, label)
+ visitInsn(RETURN)
+ visitLabel(label)
+ visitInsn(RETURN)
+ }
+ def lines = opcodeLines(bytecode)
+
+ assert lines.size() == 4
+ assert lines[0] == 'ALOAD 0'
+ assert lines[1].startsWith(expected)
+ assert lines[2..3] == ['RETURN', 'RETURN']
+ assert !lines[1].startsWith(Printer.OPCODES[opcode])
+ }
+ }
+
+ @Test
+ void removesBareDupBeforeMatchingPop() {
+ def bytecode = sequenceFor {
+ visitInsn(Opcodes.ICONST_1)
+ visitInsn(Opcodes.DUP)
+ visitInsn(Opcodes.POP)
+ visitLdcInsn(2L)
+ visitInsn(Opcodes.DUP2)
+ visitInsn(Opcodes.POP2)
+ visitInsn(RETURN)
+ }
+
+ assert opcodeLines(bytecode) == [
+ 'ICONST_1',
+ 'LDC 2L',
+ 'RETURN',
Review Comment:
Agreed — once DUP/POP is gone the constants are already flushed out of the
single load window, so a second pass / wider window would be needed to drop
them before void RETURN. Happy as a follow-up; not this PR.
> Harden and extend bytecode peephole optimization
> ------------------------------------------------
>
> Key: GROOVY-12162
> URL: https://issues.apache.org/jira/browse/GROOVY-12162
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
>
> h2. Summary
> Follow-up to GROOVY-12065. Make the single-pass peephole layer apply to
> *every* method body, restore classgen diagnostics after visitor wrapping, and
> extend safe stack-local rewrites so generated bytecode stays compact without
> changing runtime semantics.
> h2. Background
> After GROOVY-12065, {{OperandStack}} emits many constants uniformly via
> {{visitLdcInsn}} and relies on {{PeepholeOptimizingMethodVisitor}} to narrow
> them ({{ICONST_*}}, {{BIPUSH}}, {{SIPUSH}}, etc.).
> Two practical gaps remained:
> # *Incomplete coverage* — the peephole wrapper was applied only at selected
> {{visitMethod}} sites. Synthetic helpers (call-site array init, large-list
> chunks, MOP bridges, …) often bypassed it and kept non-compact forms.
> # *Broken diagnostics* — with the peephole visitor outside
> {{TraceMethodVisitor}}, {{mv instanceof TraceMethodVisitor}} failed, so "Last
> known generated bytecode" was never printed when {{visitMaxs}} failed under
> classgen logging.
> h2. Goals
> * One installation point so *all* methods (user + synthetic) get the same
> compaction.
> * Correct classgen logging / failure dumps through the wrapper chain.
> * More rewrites that stay single-pass, stack-local, and semantics-preserving.
> * Strong unit coverage for edge cases (CHECKCAST side effects, void
> {{RETURN}}, signed zeros, box/unbox mismatch).
> h2. Approach
> h3. Uniform installation
> * Add {{PeepholeOptimizingClassVisitor}}, installed once from
> {{WriterController}}, wrapping every method visitor.
> * Remove ad-hoc {{new PeepholeOptimizingMethodVisitor(...)}} from
> {{AsmClassGenerator}} and {{CallSiteWriter}}.
> * {{PeepholeOptimizingMethodVisitor.wrap(null)}} propagates {{null}}
> unchanged (ASM may skip a method body); wrap is idempotent.
> h3. Diagnostics
> * Add {{PeepholeOptimizingMethodVisitor.printTraceBytecode}} to walk nested
> peephole layers and print an inner {{TraceMethodVisitor}} when present.
> * {{AsmClassGenerator}} uses it on {{visitMaxs}} failure so classgen logging
> still shows the last known bytecode.
> h3. Extended rewrites (single basic-block window)
> Still at most one pending load, one pending box, and one pending
> {{DUP}}/store:
> * *CHECKCAST attachment* — attach to pending {{ALOAD}} or reference constant
> so {{load}}; {{CHECKCAST}}; {{POP}} can collapse when unused; keep the cast
> when the value is used.
> * *Null compare* — {{ACONST_NULL}}; {{IF_ACMPEQ}}/{{IF_ACMPNE}} →
> {{IFNULL}}/{{IFNONNULL}}.
> * *Zero compare* — {{ICONST_0}}; {{IF_ICMP*}} → {{IF*}} via the fixed JVM
> opcode offset ({{IF_ICMPEQ}}…{{IF_ICMPLE}} ↔ {{IFEQ}}…{{IFLE}}).
> * *Bare DUP / DUP+store+pop* — matching {{POP}}/{{POP2}} collapses to a plain
> store, or eliminates the dup entirely.
> * *Void RETURN* — drop dead loads/constants (preserve paired {{IINC}}); for
> pending {{CHECKCAST}} (standalone or attached), emit the cast then {{POP}} so
> the type-check side effect is kept and the void return stays verifiable.
> * *Constants* — integers re-emitted via {{BytecodeHelper.pushConstant}} on
> the *delegate* (no re-buffering); {{ConstantDynamic}} is never buffered
> (bootstrap may have side effects); signed FP zeros still compared by raw bits
> (GROOVY-9797).
> * *Box/unbox cancellation* (gjit-inspired) — matching {{Wrapper.valueOf}} +
> {{xxxValue}}, or {{DefaultTypeTransformation.box}} + {{xxxUnbox}}, cancel to
> a no-op; mismatched types are left intact.
> * *Dead box* — {{valueOf}}/{{box}} + {{POP}}/{{POP2}} drops the box and pops
> the original primitive ({{POP2}} when wide).
> * *Boolean constant folding* — {{GETSTATIC Boolean.TRUE/FALSE}} +
> {{booleanValue}}/{{booleanUnbox}} → {{ICONST_1}}/{{ICONST_0}}.
> h2. Safety
> * Flush pending state before labels, frames, line numbers, invokes (except
> matched box/unbox), switches, maxs/end, and other non-local boundaries.
> * No second compilation pass; no full data-flow analysis.
> * Observable behaviour unchanged; only denser equivalent bytecode within a
> basic block.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
