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. TestLong.java
        0.3 kB
        Elena Semukhina
      2. TestFileChannel.java
        1.0 kB
        Elena Semukhina
      3. TestLong2.java
        0.3 kB
        Egor Pasko
      4. patch.diff
        3 kB
        Nikolay Sidelnikov

        Activity

        Elena Semukhina created issue -
        Hide
        Elena Semukhina added a comment -

        test attached.

        Show
        Elena Semukhina added a comment - test attached.
        Elena Semukhina made changes -
        Field Original Value New Value
        Attachment TestLong.java [ 12342562 ]
        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.
        Elena Semukhina made changes -
        Attachment TestFileChannel.java [ 12342569 ]
        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.
        Egor Pasko made changes -
        Attachment TestLong2.java [ 12342632 ]
        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
        Nikolay Sidelnikov made changes -
        Attachment patch.diff [ 12342838 ]
        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.
        Geir Magnusson Jr made changes -
        Assignee Geir Magnusson Jr [ geir ]
        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
        Geir Magnusson Jr made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Geir Magnusson Jr made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        5d 49m 1 Geir Magnusson Jr 14/Oct/06 11:46
        Resolved Resolved Closed Closed
        14s 1 Geir Magnusson Jr 14/Oct/06 11:46

          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