[
https://issues.apache.org/jira/browse/GROOVY-12542?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18133377#comment-18133377
]
ASF GitHub Bot commented on GROOVY-12542:
-----------------------------------------
testlens-app[bot] commented on PR #3062:
URL: https://github.com/apache/groovy/pull/3062#issuecomment-6460517221
## ✅ All tests passed ✅
🏷️ Commit: 32a5321a8be2faf54f5e44f98a5cea3c75b9ac81
▶️ Tests: 123933 executed
⚪️ Checks: 32/32 completed
---
_Learn more about TestLens at
[testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
> Branch directly on comparisons in conditions instead of first producing a
> boolean value
> ---------------------------------------------------------------------------------------
>
> Key: GROOVY-12542
> URL: https://issues.apache.org/jira/browse/GROOVY-12542
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
>
> h3. Problem
> A comparison used as an {{if}}/{{while}}/{{for}} condition is first turned
> into a {{0}}/{{1}} value and then branched on, instead of jumping directly as
> javac does:
> {code:groovy}
> @groovy.transform.CompileStatic
> static void m(int n) {
> for (int i = 0; i < n; i++) { }
> }
> {code}
> {noformat}
> Groovy (current) javac (equivalent Java)
> ILOAD i ILOAD i
> ILOAD n ILOAD n
> IF_ICMPGE L1 IF_ICMPGE EXIT
> ICONST_1
> GOTO L2
> L1: ICONST_0
> L2: IFEQ EXIT
> {noformat}
> The comparison code ({{BinaryExpressionWriter.writeStdCompare}} and related
> helpers) always produces a boolean, and the statement writers then emit
> {{IFEQ}}/{{IFNE}} on it. This affects dynamic and statically compiled code.
> The peephole optimizer cannot help, because the pattern spans labels.
> Besides the extra instructions and branches, some bytecode consumers only
> recognise javac's shape. For example, TornadoVM's TC compiler
> ({{CountedLoop}} in tc-ir) requires a loop header to end in {{cond_br (icmp
> iv, bound)}}. With the extra boolean step it finds no loop bound, so
> {{@Parallel}} loops written in Groovy fail with:
> {noformat}
> TC-E3103 the bounds of the @Parallel loops are not compile-time constants
> after specialisation, so no launch domain can be derived
> {noformat}
> h3. Proposed change
> Add a small {{MethodVisitor}} stage, {{ConditionalJumpFoldingMethodVisitor}},
> installed by {{PeepholeOptimizingClassVisitor}} directly under
> {{PeepholeOptimizingMethodVisitor}}. It rewrites:
> {noformat}
> Jcc L1; ICONST_1; GOTO L2; L1: ICONST_0; L2: IFEQ L3 ==> Jcc L3
> Jcc L1; ICONST_1; GOTO L2; L1: ICONST_0; L2: IFNE L3 ==> !Jcc L3
> {noformat}
> for any conditional jump ({{IFxx}}, {{IF_ICMPxx}}, {{IF_ACMPxx}},
> {{IFNULL}}/{{IFNONNULL}}). Partial matches are replayed unchanged.
> Safety:
> * *Exact semantics:* the folded jump tests the same operands, or the same
> {{LCMP}}/{{FCMPx}}/{{DCMPx}} result, as the original, so NaN behaviour is
> unchanged.
> * *Shared labels untouched:* the fold only fires when {{L1}} and {{L2}} are
> each referenced solely by the pattern, so labels shared with other jumps
> (e.g. from {{&&}}/{{||}}) are left as is.
> * *Fails loudly:* if a removed label were ever referenced later, a
> {{GroovyBugError}} is raised rather than emitting wrong bytecode.
> * *Stored values unaffected:* a materialized boolean that is stored rather
> than branched on (e.g. {{boolean r = a < b}}) is not touched.
> * *Frames:* classes are written with {{COMPUTE_FRAMES}}, so no stack map
> frames need adjusting.
> * {{PeepholeOptimizingMethodVisitor.printTraceBytecode}} now unwraps the new
> stage too.
> h3. Tests
> * New {{ConditionalJumpFoldingMethodVisitorTest}}:
> ** bytecode shape for {{for}} and {{if}} conditions
> ** stored comparisons are not folded
> ** runtime results in dynamic and {{@CompileStatic}} code for {{!}}, {{&&}},
> {{||}}, ternary, int/long/float/double, null and identity comparisons
> ** for NaN, a folded branch always agrees with the same comparison stored as
> a value
> * Updated expected bytecode in
> {{StaticCompilationTest.testSingleAssignment3}},
> {{StaticCompileNullCompareOptimizationTest.testOptimizeGroovyTruthForPrimitiveBoolean2}}
> and {{testOptimizeGroovyTruthForPrimitiveNumberType}}, which asserted the
> old shape.
> * Full core test suite: 18,659 tests, 0 failures.
> h3. Note
> Relates to GROOVY-12538 (primitive double/float {{>}} and {{>=}} return true
> for NaN). That fix changes which compare instruction ({{DCMPL}}/{{DCMPG}},
> {{FCMPL}}/{{FCMPG}}) precedes the conditional jump. This change only rewrites
> from the jump onwards, so it preserves whichever instruction is chosen.
> Tested together: with PR #3053 applied on top of this change,
> {{DoubleOperationsTest}} and {{ConditionalJumpFoldingMethodVisitorTest}} both
> pass.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
