Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK11 JDK17 Different Result from Xint #15534

Closed
connglli opened this issue Jul 13, 2022 · 11 comments
Closed

JDK11 JDK17 Different Result from Xint #15534

connglli opened this issue Jul 13, 2022 · 11 comments

Comments

@connglli
Copy link

Java -version output

openjdk version "11.0.16-internal" 2022-07-19
OpenJDK Runtime Environment (build 11.0.16-internal+0-adhoc..openj9-openjdk-jdk11)
Eclipse OpenJ9 VM (build master-4ca209b54, JRE 11 Linux amd64-64-Bit Compressed References 20220615_000000 (JIT enabled, AOT enabled)
OpenJ9   - 4ca209b54
OMR      - 26b89f9f9
JCL      - 231dcc9eeb based on jdk-11.0.16+6)

openjdk version "17.0.4-internal" 2022-07-19
OpenJDK Runtime Environment (build 17.0.4-internal+0-adhoc..openj9-openjdk-jdk17)
Eclipse OpenJ9 VM (build master-79f0b73fa, JRE 17 Linux amd64-64-Bit Compressed References 20220701_000000 (JIT enabled, AOT enabled)
OpenJ9   - 79f0b73fa
OMR      - d018241d7
JCL      - c6e2f71170b based on jdk-17.0.4+7)

Summary of problem

The following test is a bit tricky. It gives a different result from -Xint. But cannot understand which JIT level causes the test to fail. So it should be different from #15306 (from scorching) and #15347 (from hot).

It affects both JDK11 and JDK17.

class Test {
  int N;
  long instanceCount;
  float fFld;
  long vMeth1_check_sum;

  void vMeth1() {
    if (ax$0) {
      String[][] ax$3 = new String[2515][1];
      for (String[] ax$5 : ax$3) {}
      return;
    }
    int i2, i3, i5, i6 = 2, i7 = 227;
    for (i3 = 6; i3 < 157; ++i3)
      for (i5 = 4; i5 > 1; --i5) {
        instanceCount = i7;
        i6 += i7;
      }
    vMeth1_check_sum += i6;
  }

  void vMeth() {
    try {
      ax$0 = true;
      for (int ax$7 = 0; ax$7 < 5913; ax$7 += 1) vMeth1();
    } finally {
      ax$0 = false;
    }
    vMeth1();
  }

  int iMeth() {
    long[] lArr = new long[N];
    vMeth();
    long meth_res = checkSum(lArr);
    return (int) meth_res;
  }

  void mainTest(String[] strArr1) {
    fFld -= iMeth();
    System.out.println(vMeth1_check_sum);
  }

  public static void main(String[] strArr) {
    Test _instance = new Test();
    _instance.mainTest(strArr);
  }

  public static long checkSum(long[] a) {
      long sum = 0;
      for (int j = 0; j < a.length; j++) {
          sum += (a[j] / (j + 1) + a[j] % (j + 1));
      }
      return sum;
  }

  Boolean ax$0;
}

Results of OpenJ9-Xint (HotSpot/ART also give the same result):

$ ./OpenJ9/jdk11/java -Xint Test
102833

Results of OpenJ9:

$ ./OpenJ9/jdk11/java Test
34279

Diagnosis

Only two methods in Test are compiled:

$ ./OpenJ9/jdk17/bin/java '-Xjit:verbose' Test 2>&1 | grep Test.                     
+ (warm) Test.vMeth1()V @ 00007F28659DA3C0-00007F28659DA5F8 OrdinaryMethod - Q_SZ=0 Q_SZI=0 QW=12 j9m=000000000018F488 bcsz=110 GCR compThreadID=0 CpuLoad=18%(2%avg) JvmCpu=0%
+ (profiled very-hot) Test.vMeth1()V @ 00007F28659DA640-00007F28659DA94A OrdinaryMethod 100.00% T Q_SZ=0 Q_SZI=0 QW=100 j9m=000000000018F488 bcsz=110 OSR JPROF compThreadID=0 CpuLoad=18%(2%avg) JvmCpu=0%
+ (scorching) Test.vMeth1()V @ 00007F28659DA9A0-00007F28659DAB8C OrdinaryMethod J Q_SZ=0 Q_SZI=0 QW=100 j9m=000000000018F488 bcsz=110 OSR compThreadID=0 CpuLoad=18%(2%avg) JvmCpu=0%
+ (warm) Test.vMeth()V @ 00007F28659DABE0-00007F28659DAEBC OrdinaryMethod - Q_SZ=0 Q_SZI=0 QW=12 j9m=000000000018F4A8 bcsz=54 GCR compThreadID=0 CpuLoad=18%(2%avg) JvmCpu=0%
+ (hot) Test.vMeth()V @ 00007F28659DA674-00007F28659DA8AF MethodInProgress - Q_SZ=0 Q_SZI=0 QW=1 j9m=000000000018F4A8 bcsz=54 DLT@10 compThreadID=0 CpuLoad=18%(2%avg) JvmCpu=0%

It seems the compilation of Test.vMeth1() causes this failure because if you exclude it the result becomes correct (excluding Test.vMeth() makes no effects):

$ ./OpenJ9/jdk17/bin/java '-Xjit:exclude={Test.vMeth1()V}' Test
102833


$ ./OpenJ9/jdk17/bin/java '-Xjit:exclude={Test.vMeth()V}' Test
34279

But with -Xjit option, no matter which JIT level you try, the bug never reappears:

$ ./OpenJ9/jdk17/bin/java '-Xjit:{Test.vMeth1()V}(optlevel=noopt)' Test 
102833
$ ./OpenJ9/jdk17/bin/java '-Xjit:{Test.vMeth1()V}(optlevel=cold)' Test 
102833
$ ./OpenJ9/jdk17/bin/java '-Xjit:{Test.vMeth1()V}(optlevel=warm)' Test
102833
$ ./OpenJ9/jdk17/bin/java '-Xjit:{Test.vMeth1()V}(optlevel=hot)' Test 
102833
$ ./OpenJ9/jdk17/bin/java '-Xjit:{Test.vMeth1()V}(optlevel=veryhot)' Test
102833
$ ./OpenJ9/jdk17/bin/java '-Xjit:{Test.vMeth1()V}(optlevel=scorching)' Test
102833

As a comparison, 15306 can reproduce its bug from scorching and 15347 from hot. So I guess this is a different new one.

@connglli
Copy link
Author

If there are other options that are helpful for me to diagnose new problems, feel free to let me know.

@connglli
Copy link
Author

Perhaps it fails when JIT compiler optimizes vMeth1() from one level to another level? So directly set the optlevel to any specific one cannot reproduce the bug?

@pshipton
Copy link
Member

@0xdaryl fyi

@tajila
Copy link
Contributor

tajila commented Aug 24, 2022

@hzongaro

@hzongaro
Copy link
Member

Running the test with the option '-Xjit:{Test.vMeth1()V}(disableSequenceSimplification)' produces the correct result. This might be a duplicate of #15349 and #15306.

As the problem is easily reproduced, and we have a pretty good idea which optimization is causing the problem, I think we can keep this targeted for 0.35.

@hzongaro
Copy link
Member

I've been experimenting with a fix for this problem. Unfortunately, although it seems to fix this issue, it doesn't fix #15306, so there might be more than one distinct problem in this optimization.

As any fix has the potential to affect performance and correctness, it might be safer to defer this from the 0.35 release so we aren't rushing in a fix. @pshipton

@hzongaro
Copy link
Member

I was mistaken about #15306. The fix that I was experimenting with for this issue does appear to fix #15306 as well.

@hzongaro
Copy link
Member

hzongaro commented Oct 3, 2022

Duplicate of #15306

@hzongaro hzongaro marked this as a duplicate of #15306 Oct 4, 2022
@connglli
Copy link
Author

connglli commented Oct 4, 2022

@hzongaro Could you tell me which test case (the original one or the one commented in 27 July) this one is duplicate to? And do I need to open a new thread for the one commented in 27 July?

@hzongaro
Copy link
Member

hzongaro commented Oct 4, 2022

Could you tell me which test case (#15306 (comment) or #15306 (comment)) this one is duplicate to?

@connglli It was the original one that this duplicates. Sorry for not being clear!

And do I need to open a new thread for the one commented in 27 July?

No need to open a new issue - the proposed changes in OMR pull request eclipse/omr#6747 fix both of those problems.

@hzongaro
Copy link
Member

Fixed by OMR pull request eclipse/omr#6747

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants