Harmony
  1. Harmony
  2. HARMONY-1774

[drlvm][unit] Classlib test org.apache.harmony.luni.tests.java.lang.LongTest fails in OPT mode

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: DRLVM
    • Labels:
      None

      Description

      5 test methods in this test fail OPT mode while pass in JET.

      > java -showversion
      Apache Harmony Launcher : (c) Copyright 1991, 2006 The Apache Software Foundatio
      n or its licensors, as applicable.
      java version "1.5.0"
      pre-alpha : not complete or compatible
      svn = r454267, (Oct 9 2006), Windows/ia32/msvc 1310, debug build
      http://incubator.apache.org/harmony

      I'll attach a simple test to reproduce the issue.

      1. patch.diff
        3 kB
        Nikolay Sidelnikov
      2. TestLong2.java
        0.3 kB
        Egor Pasko
      3. TestFileChannel.java
        1.0 kB
        Elena Semukhina
      4. TestLong.java
        0.3 kB
        Elena Semukhina

        Activity

        Hide
        Elena Semukhina added a comment -

        test attached.

        Show
        Elena Semukhina added a comment - test attached.
        Hide
        Elena Semukhina added a comment -

        org.apache.harmony.nio.tests.java.nio.channels.FileChannelTest and org.apache.harmony.nio.tests.java.nio.channels.FileLockTest seem to fail due to this issue as well.

        I attached one more test to reproduce the issue.

        If it is another issue, a separate JIRA has to be filed.

        Show
        Elena Semukhina added a comment - org.apache.harmony.nio.tests.java.nio.channels.FileChannelTest and org.apache.harmony.nio.tests.java.nio.channels.FileLockTest seem to fail due to this issue as well. I attached one more test to reproduce the issue. If it is another issue, a separate JIRA has to be filed.
        Hide
        Elena Semukhina added a comment -
        Show
        Elena Semukhina added a comment - [drlvm] [unit] Blocked http://wiki.apache.org/harmony/Unit_Tests_Pass_on_DRLVM
        Hide
        Egor Pasko added a comment -

        passes on JET, failes on OPT with:
        $HARMONY -Xem:opt TestLong
        Failed to throw exception for MAX_VALUE + 1

        I'll take a look

        Show
        Egor Pasko added a comment - passes on JET, failes on OPT with: $HARMONY -Xem:opt TestLong Failed to throw exception for MAX_VALUE + 1 I'll take a look
        Hide
        Egor Pasko added a comment -

        j.l.Long::parse is compiled incorrectly by OPT, will reduce

        Show
        Egor Pasko added a comment - j.l.Long::parse is compiled incorrectly by OPT, will reduce
        Hide
        Egor Pasko added a comment -

        TestLong2 is a very simple reproducer. Passes on JET, fails on OPT. Looks like a bug in Simplifier or reassoc.

        Show
        Egor Pasko added a comment - TestLong2 is a very simple reproducer. Passes on JET, fails on OPT. Looks like a bug in Simplifier or reassoc.
        Hide
        Egor Pasko added a comment -

        I found even a shorter test:
        public class TestLong2
        {
        public static void main(String[] args)

        { parse(-(Long.MIN_VALUE + 8)); }

        private static void parse(long rs)
        {
        long result = rs + 8;
        if (result < 0)

        { System.out.println("PASS"); return; }

        System.out.println("FAIL");
        }
        }

        OPT.

        {ranslator and optimizer}

        parts are innocent here, the bug is in handling the branch ((long + 8) < 0) in code generator:
        addl $8, %ecx
        adcl $0, %eax
        jnz .X
        jb
        .X:
        jnl

        I am slightly confused with all these zero jumps, and they look suspicious to me.
        In case of "rs" instead of "rs + 8" in the above test, CG generates an explicit CMP instruction before JNZ and the test passes. Can it be the reason? CG gurus, can you look at this bug?

        OFFTOPIC:
        in that case GCC produces something like:
        addl $8, %eax
        adcl $0, %edx
        testl %edx, %edx
        js

        this should be faster. Can we do something similar in CG in the nearest future?

        Show
        Egor Pasko added a comment - I found even a shorter test: public class TestLong2 { public static void main(String[] args) { parse(-(Long.MIN_VALUE + 8)); } private static void parse(long rs) { long result = rs + 8; if (result < 0) { System.out.println("PASS"); return; } System.out.println("FAIL"); } } OPT. {ranslator and optimizer} parts are innocent here, the bug is in handling the branch ((long + 8) < 0) in code generator: addl $8, %ecx adcl $0, %eax jnz .X jb .X: jnl I am slightly confused with all these zero jumps, and they look suspicious to me. In case of "rs" instead of "rs + 8" in the above test, CG generates an explicit CMP instruction before JNZ and the test passes . Can it be the reason? CG gurus, can you look at this bug? OFFTOPIC: in that case GCC produces something like: addl $8, %eax adcl $0, %edx testl %edx, %edx js this should be faster. Can we do something similar in CG in the nearest future?
        Hide
        Nikolay Sidelnikov added a comment -

        The problem is in RCE (redundant comparison elimination) optimization and I8Lowerer incompatibility.

        I8Lowerer produces two jumps depend on one comparison. RCE takes into account only first of it and consider comparison as redundant.

        I propose to fix I8Lowerer to make produced code to be more unified for further optimizations.

        Show
        Nikolay Sidelnikov added a comment - The problem is in RCE (redundant comparison elimination) optimization and I8Lowerer incompatibility. I8Lowerer produces two jumps depend on one comparison. RCE takes into account only first of it and consider comparison as redundant. I propose to fix I8Lowerer to make produced code to be more unified for further optimizations.
        Hide
        Egor Pasko added a comment -

        Nikolay, please, take HARMONY-1775 into account, it fails due to RCE too, but it has nothing about i8lowerer. So, I propose to fix RCE

        Show
        Egor Pasko added a comment - Nikolay, please, take HARMONY-1775 into account, it fails due to RCE too, but it has nothing about i8lowerer. So, I propose to fix RCE
        Hide
        Egor Pasko added a comment -

        TestFileChannel.java is the same issue, so, all Jitrino.OPT-specific failures have the same root cause

        Show
        Egor Pasko added a comment - TestFileChannel.java is the same issue, so, all Jitrino.OPT-specific failures have the same root cause
        Hide
        Nikolay Sidelnikov added a comment -

        The bug was fixed from both sides: I8Lowerer and RCE

        Show
        Nikolay Sidelnikov added a comment - The bug was fixed from both sides: I8Lowerer and RCE
        Hide
        Nikolay Sidelnikov added a comment -

        Some clarifications for latest patch:

        RCE:
        Now basic blocks are examined in post-order mode. It provides proper search of conditional instructions depend on one comparison. When such pattern is found searching of comparison to remove is aborted (for particular conditional instructions)

        I8Lowerer:
        New comparison instruction added to compare high dwords of source operands to escape situation when two conditional instructions depend on one comparison.

        Show
        Nikolay Sidelnikov added a comment - Some clarifications for latest patch: RCE: Now basic blocks are examined in post-order mode. It provides proper search of conditional instructions depend on one comparison. When such pattern is found searching of comparison to remove is aborted (for particular conditional instructions) I8Lowerer: New comparison instruction added to compare high dwords of source operands to escape situation when two conditional instructions depend on one comparison.
        Hide
        Geir Magnusson Jr added a comment -

        r463917

        Ubuntu 6- smoke, c-unit, ~kernel

        Show
        Geir Magnusson Jr added a comment - r463917 Ubuntu 6- smoke, c-unit, ~kernel

          People

          • Assignee:
            Geir Magnusson Jr
            Reporter:
            Elena Semukhina
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development