[
https://issues.apache.org/jira/browse/GROOVY-12542?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul King updated GROOVY-12542:
-------------------------------
Description:
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.
was:
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
While testing, I noticed that {{Double.NaN >= 1d}} and {{1d >= Double.NaN}}
both evaluate to {{true}} in Groovy (dynamic and {{@CompileStatic}}, also on
6.0.0-beta-2), whereas IEEE 754 and Java give {{false}}. This is existing
behaviour and is preserved by this change; it may warrant a separate issue.
> 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)
