Paul King created GROOVY-12542:
----------------------------------
Summary: 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
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.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
