8265917: Different values computed by C2 and interpreter/C1 for Math.pow(x, 2.0) on x86_32 #3677

DamonFool · 2021-04-25T11:39:43Z

Hi all,

C2 may produce different results for Math.pow(x, 2.0) compared with interpreter/C1 on x86_32.

E.g., for Math.pow(1.0 / 2047, 2.0):

interpreter: 2.38651580386563E-7 
         C2: 2.3865158038656307E-7

The reason is that C2 will replace Math.pow(x, 2.0) with x*x.
However, there is no such optimization for interpreter/C1 on x86_32.

The fix just disables C2's opt for Math.pow(x, 2.0) on x86_32 since nobody (or very few people) would run x86_32.
And we don't have a plan to implement such opt on x86_32.

Another reason to fix this bug is that we need this patch to extend C2's opt for Math.pow(x, 0.5) on other platforms.

Thanks.
Best regards,
Jie

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8265917: Different values computed by C2 and interpreter/C1 for Math.pow(x, 2.0) on x86_32

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3677/head:pull/3677
$ git checkout pull/3677

Update a local copy of the PR:
$ git checkout pull/3677
$ git pull https://git.openjdk.java.net/jdk pull/3677/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3677

View PR using the GUI difftool:
$ git pr show -t 3677

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3677.diff

…pow(x, 2.0) on x86_32

bridgekeeper · 2021-04-25T11:40:24Z

👋 Welcome back jiefu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

DamonFool · 2021-04-25T11:40:51Z

/test
/label add hotspot-compiler
/cc hotspot-compiler

openjdk · 2021-04-25T11:42:31Z

@DamonFool
The hotspot-compiler label was successfully added.

openjdk · 2021-04-25T11:43:39Z

@DamonFool The hotspot-compiler label was already applied.

mlbridge · 2021-04-25T11:46:04Z

Webrevs

vnkozlov · 2021-04-25T18:09:01Z

I think you should fix 32-bit version of dpow stub because 64-bit stub has this optimization:
https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/x86/macroAssembler_x86_pow.cpp#L828

Also add pow(x, 0.5) as well.

C dpow code in VM has this optimization which works in 32- and in 64-bit when stub is not used (for example, on other platforms):
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/sharedRuntimeTrans.cpp#L496

Java lib code also has this optimization:
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/FdLibm.java#L362

Only 32-bit x86 stub is missing it:
https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/x86/macroAssembler_x86_pow.cpp#L2544

The stub is generated only on x86:

$ grep -r StubRoutines::_dpow src/hotspot/cpu/
src/hotspot/cpu//x86/stubGenerator_x86_64.cpp:        StubRoutines::_dpow = generate_libmPow();
src/hotspot/cpu//x86/stubGenerator_x86_32.cpp:        StubRoutines::_dpow = generate_libmPow();

I suggest to fix it to be consistent with the rest of VM and Java lib code.

DamonFool · 2021-04-26T06:17:50Z

I think you should fix 32-bit version of dpow stub because 64-bit stub has this optimization:
https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/x86/macroAssembler_x86_pow.cpp#L828

Also add pow(x, 0.5) as well.

C dpow code in VM has this optimization which works in 32- and in 64-bit when stub is not used (for example, on other platforms):
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/sharedRuntimeTrans.cpp#L496

Java lib code also has this optimization:
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/FdLibm.java#L362

Only 32-bit x86 stub is missing it:
https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/x86/macroAssembler_x86_pow.cpp#L2544

The stub is generated only on x86:
$ grep -r StubRoutines::_dpow src/hotspot/cpu/
src/hotspot/cpu//x86/stubGenerator_x86_64.cpp:        StubRoutines::_dpow = generate_libmPow();
src/hotspot/cpu//x86/stubGenerator_x86_32.cpp:        StubRoutines::_dpow = generate_libmPow();
I suggest to fix it to be consistent with the rest of VM and Java lib code.

Hi @vnkozlov ,

Math.pow(x, 2.0) has been optimized for x86_32.
As for the Math.pow(x, 0.5), it will be handled in JDK-8265940 after this issue.

Thanks.

DamonFool · 2021-04-26T06:21:14Z

Testing:
-tier1 ~ tier3 on Linux/x86_32, no regression

vnkozlov

Good.

vnkozlov · 2021-04-26T16:08:19Z

As for the Math.pow(x, 0.5), it will be handled in JDK-8265940 after this issue.

Okay.

openjdk · 2021-04-26T17:32:03Z

@DamonFool This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8265917: Different values computed by C2 and interpreter/C1 for Math.pow(x, 2.0) on x86_32

Reviewed-by: kvn, thartmann

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 58 new commits pushed to the master branch:

e144104: 8262908: JFR: Allow JFR to stream events from a known repository path
30b1354: 8266153: mark hotspot compiler/onSpinWait tests which ignore VM flags
eeddb30: 8266150: mark hotspot compiler/arguments tests which ignore VM flags
feb18d2: 8266149: mark hotspot compiler/startup tests which ignore VM flags
eb72950: 8266154: mark hotspot compiler/oracle tests which ignore VM flags
7f4a9f6: 8266088: compiler/arguments/TestPrintOptoAssemblyLineNumbers test should user driver mode
f560b89: 8264873: Dependencies: Split ClassHierarchyWalker
0a88f0a: 8255915: jdk/incubator/vector/AddTest.java timed out
ab2aec2: 8265938: C2's conditional move optimization does not handle top Phi
5634f20: 8265932: Move safepoint related fields from class Thread to JavaThread
... and 48 more: https://git.openjdk.java.net/jdk/compare/f1f2afda5a0ab8f213e8a1b5324a251928c8d81a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

DamonFool · 2021-04-26T23:22:42Z

Good.

Thanks @vnkozlov for your review.

Let's wait for a second review of this change.
Thanks.

TobiHartmann · 2021-04-27T09:28:38Z

src/hotspot/cpu/x86/macroAssembler_x86_pow.cpp


  assert_different_registers(tmp, eax, ecx, edx);

  address static_const_table_pow = (address)_static_const_table_pow;
+  static double DOUBLE2 = 2.0;


Constants in the 64-bit code and _static_const_table_pow above use ATTRIBUTE_ALIGNED. Should we add that for DOUBLE2 as well?

Hi @TobiHartmann ,

Thanks for your review.

Yes, it seem better to be with ATTRIBUTE_ALIGNED.
Updated.

TobiHartmann · 2021-04-27T09:29:28Z

src/hotspot/cpu/x86/macroAssembler_x86_pow.cpp


  bind(start);
  subl(rsp, 120);
  movl(Address(rsp, 64), tmp);
  lea(tmp, ExternalAddress(static_const_table_pow));
  movsd(xmm0, Address(rsp, 128));
  movsd(xmm1, Address(rsp, 136));
+
+  // Special case: pow(x, 2.0) => x * x


64-bit code also handles Special case: pow(x, 0.5) => sqrt(x). Don't we need to add that as well for consistency?

64-bit code also handles Special case: pow(x, 0.5) => sqrt(x). Don't we need to add that as well for consistency?

This is the bug fix for C2's optimization of pow(x, 2.0).

pow(x, 0.5) will be handled in JDK-8265940 .
Thanks.

TobiHartmann

Looks good to me.

DamonFool · 2021-04-27T23:36:30Z

Looks good to me.

Thanks @TobiHartmann .

@vnkozlov , are you OK with the latest change?
Thanks.

vnkozlov

Latest version with alignment is good.

DamonFool · 2021-04-28T03:09:21Z

Latest version with alignment is good.

Thanks @vnkozlov .
/integrate

openjdk · 2021-04-28T03:10:50Z

@DamonFool Since your change was applied there have been 58 commits pushed to the master branch:

e144104: 8262908: JFR: Allow JFR to stream events from a known repository path
30b1354: 8266153: mark hotspot compiler/onSpinWait tests which ignore VM flags
eeddb30: 8266150: mark hotspot compiler/arguments tests which ignore VM flags
feb18d2: 8266149: mark hotspot compiler/startup tests which ignore VM flags
eb72950: 8266154: mark hotspot compiler/oracle tests which ignore VM flags
7f4a9f6: 8266088: compiler/arguments/TestPrintOptoAssemblyLineNumbers test should user driver mode
f560b89: 8264873: Dependencies: Split ClassHierarchyWalker
0a88f0a: 8255915: jdk/incubator/vector/AddTest.java timed out
ab2aec2: 8265938: C2's conditional move optimization does not handle top Phi
5634f20: 8265932: Move safepoint related fields from class Thread to JavaThread
... and 48 more: https://git.openjdk.java.net/jdk/compare/f1f2afda5a0ab8f213e8a1b5324a251928c8d81a...master

Your commit was automatically rebased without conflicts.

Pushed as commit 889d246.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8265917: Different values computed by C2 and interpreter/C1 for Math.…

a7d32c2

…pow(x, 2.0) on x86_32

openjdk bot added rfr Pull request is ready for review hotspot-compiler hotspot-compiler-dev@openjdk.org labels Apr 25, 2021

Optimize pow(x, 2.0) for x86_32

dde5975

vnkozlov approved these changes Apr 26, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 26, 2021

TobiHartmann reviewed Apr 27, 2021

View reviewed changes

Add align attribute for 2.0

9c4283e

TobiHartmann approved these changes Apr 27, 2021

View reviewed changes

vnkozlov approved these changes Apr 28, 2021

View reviewed changes

openjdk bot closed this Apr 28, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 28, 2021

DamonFool deleted the JDK-8265917 branch April 28, 2021 03:11

8265917: Different values computed by C2 and interpreter/C1 for Math.pow(x, 2.0) on x86_32 #3677

8265917: Different values computed by C2 and interpreter/C1 for Math.pow(x, 2.0) on x86_32 #3677

DamonFool commented Apr 25, 2021 •

edited by openjdk bot

bridgekeeper bot commented Apr 25, 2021

DamonFool commented Apr 25, 2021

openjdk bot commented Apr 25, 2021

openjdk bot commented Apr 25, 2021

mlbridge bot commented Apr 25, 2021 •

edited

vnkozlov commented Apr 25, 2021

DamonFool commented Apr 26, 2021

DamonFool commented Apr 26, 2021

vnkozlov left a comment

vnkozlov commented Apr 26, 2021

openjdk bot commented Apr 26, 2021 •

edited

DamonFool commented Apr 26, 2021

TobiHartmann Apr 27, 2021

DamonFool Apr 27, 2021

TobiHartmann Apr 27, 2021

DamonFool Apr 27, 2021

TobiHartmann left a comment

DamonFool commented Apr 27, 2021

vnkozlov left a comment

DamonFool commented Apr 28, 2021

openjdk bot commented Apr 28, 2021

8265917: Different values computed by C2 and interpreter/C1 for Math.pow(x, 2.0) on x86_32 #3677

8265917: Different values computed by C2 and interpreter/C1 for Math.pow(x, 2.0) on x86_32 #3677

Conversation

DamonFool commented Apr 25, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Apr 25, 2021

DamonFool commented Apr 25, 2021

openjdk bot commented Apr 25, 2021

openjdk bot commented Apr 25, 2021

mlbridge bot commented Apr 25, 2021 • edited

Webrevs

vnkozlov commented Apr 25, 2021

DamonFool commented Apr 26, 2021

DamonFool commented Apr 26, 2021

vnkozlov left a comment

Choose a reason for hiding this comment

vnkozlov commented Apr 26, 2021

openjdk bot commented Apr 26, 2021 • edited

DamonFool commented Apr 26, 2021

TobiHartmann Apr 27, 2021

Choose a reason for hiding this comment

DamonFool Apr 27, 2021

Choose a reason for hiding this comment

TobiHartmann Apr 27, 2021

Choose a reason for hiding this comment

DamonFool Apr 27, 2021

Choose a reason for hiding this comment

TobiHartmann left a comment

Choose a reason for hiding this comment

DamonFool commented Apr 27, 2021

vnkozlov left a comment

Choose a reason for hiding this comment

DamonFool commented Apr 28, 2021

openjdk bot commented Apr 28, 2021

DamonFool commented Apr 25, 2021 •

edited by openjdk bot

mlbridge bot commented Apr 25, 2021 •

edited

openjdk bot commented Apr 26, 2021 •

edited