JDK-8298813: [C2] Converting double to float cause a loss of precision and resulting crypto.aes scores fluctuate #11685

sunny868 · 2022-12-15T02:52:51Z

Hi all,
For C2, convert double to float cause a loss of precision,


./chaitin.cpp:221
_high_frequency_lrg = MIN2(double(OPTO_LRG_HIGH_FREQ), _cfg.get_outer_loop_frequency());

Here, _high_frequency_lrg type is float, so maybe has a loss of precision. when it be used:


./coalesce.cpp:379
if( lrg._maxfreq >= _phc.high_frequency_lrg() ) {
   ...
}

Here, lrg._maxfreq type is double, so _high_frequency_lrg will be convert double again. But now, due to the loss of precision of _high_frequency_lrg, the conditions here may be true or false.

There are two cases that I tested for SPECjvm2008 crypto.aes.
case 1:


//chaitin.cpp:221
// fcvt.s.d $f0,$f0 #double->float
d = 16.994714324523816
f = 16.9947147

//coalesce.cpp:379
// fcvt.d.s $f0,$f0 #float->double
// fcmp.sle.d $fcc2,$f0,$f1
(gdb) i r fa0
fa0 {f = 0x0, d = 0x10} {f = -1.08420217e-19, d = 16.994714736938477}
(gdb) i r fa1
fa1 {f = 0x0, d = 0x10} {f = -7.68722312e-24, d = 16.994714324523816}

case2:


//chaitin.cpp:221
// fcvt.s.d $f0,$f0
d = 16.996332681816536
f = 16.9963322

//coalesce.cpp
// fcvt.d.s $f0,$f0
// fcmp.sle.d $fcc2,$f0,$f1
(gdb) i r fa0
fa0 {f = 0x0, d = 0x10} {f = -1.08420217e-19, d = 16.996332168579102}
(gdb) i r fa1
fa1 {f = 0x0, d = 0x10} {f = -1.73570044e-14, d = 16.996332681816536}

The above two cases result in different block generation（case2 can insert new SpillCopyNodes), and resulting score on cryto.aes is fluctuate.

This is a patch to fix this problem. Please help review it.

Thanks,
Sun Guoyun

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8298813: [C2] Converting double to float cause a loss of precision and resulting crypto.aes scores fluctuate

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer) ⚠️ Review applies to f017e49c

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11685/head:pull/11685
$ git checkout pull/11685

Update a local copy of the PR:
$ git checkout pull/11685
$ git pull https://git.openjdk.org/jdk pull/11685/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11685

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11685.diff

…d resulting crypto.aes scores fluctuate

bridgekeeper · 2022-12-15T02:54:21Z

👋 Welcome back sunny868! 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.

openjdk · 2022-12-15T02:57:54Z

@sunny868 The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-12-15T03:01:49Z

Webrevs

01: Full - Incremental (c2bf1d76)
00: Full (f017e49c)

sunny868 · 2022-12-15T09:50:04Z

The test FAIL and ERROR items has nothing to do with this patch.

TobiHartmann

This looks reasonable to me but I'm not an expert in that area. I'll run some performance testing for sanity and report back once it passed.

TobiHartmann · 2022-12-16T12:14:31Z

src/hotspot/share/opto/chaitin.hpp

@@ -478,7 +478,7 @@ class PhaseChaitin : public PhaseRegAlloc {

  Block **_blks;                // Array of blocks sorted by frequency for coalescing

-  float _high_frequency_lrg;    // Frequency at which LRG will be spilled for debug info
+  double _high_frequency_lrg;    // Frequency at which LRG will be spilled for debug info


Suggested change

double _high_frequency_lrg; // Frequency at which LRG will be spilled for debug info

double _high_frequency_lrg; // Frequency at which LRG will be spilled for debug info

sunny868 · 2022-12-17T01:23:09Z

OK. you can focus on the instruction generation of function com.sun.crypto.provider.DESCrypt::cipherBlock, sometimes it is


// result 63.4 ops/m
// B5->B112->B55

   B5: #   out( B112 B6 ) <- in( B4 )  Freq: 0.999996
    BRge   T0, A6, B112 #@branchConIU_reg_reg_short  P=0.000001 C=-1.000000
   ...
   B55: #  out( B56 ) <- in( B112 B53 B54 )  Freq: 3.54712e-05
    st_w    T6, [SP + #40]  # spill 9
    CALL,static #@CallStaticJavaDirect  wrapper for: uncommon_trap(reason='range_check' action='make_not_entrant' debug_id='0')
   ILLTRAP   ;#@ShouldNotReachHere
   ...
  B112: # out( B55 ) <- in( B5 )  Freq: 1.01327e-06
  mov    T6, #0 #@loadConI
  JMP    B55 #@jmpDir_short

and sometimes it is


// result: 41.99 ops/m
// B5->B55

     B5: #   out( B55 B6 ) <- in( B4 )  Freq: 0.999996
     mov    A6, #0 #@loadConI
     BRge   T8, T6, B55 #@branchConIU_reg_reg_short  P=0.000001 C=-1.000000

     B55: #  out( B56 ) <- in( B112 B53 B54 )  Freq: 3.54712e-05
             st_w    A6, [SP + #4]  # spill 9
     CALL,static #@CallStaticJavaDirect  wrapper for: uncommon_trap(reason='range_check' action='make_not_entrant' debug_id='0')
     ILLTRAP   ;#@ShouldNotReachHere

sunny868 · 2022-12-17T01:27:34Z

And in order to eliminate the fluctuation of running scores caused by C1, it is recommended that you use -XX:-TieredCompilation

sunny868 · 2022-12-17T01:56:56Z

By the way, adding the following changes to this patch will allow crypto.aes to get a stable high score.
But I'm not sure if this change can be committed with this patch.


diff --git a/src/hotspot/share/opto/coalesce.cpp b/src/hotspot/share/opto/coalesce.cpp
index b95987c4b09..82fbc72ad7e 100644                                           
--- a/src/hotspot/share/opto/coalesce.cpp                                       
+++ b/src/hotspot/share/opto/coalesce.cpp                                       
@@ -376,7 +376,7 @@ void PhaseAggressiveCoalesce::insert_copies( Matcher &matcher ) {
             LRG &lrg = lrgs(nidx);                                             
                                                                                
             // If this lrg has a high frequency use/def                        
-            if( lrg._maxfreq >= _phc.high_frequency_lrg() ) {                  
+            if( lrg._maxfreq > _phc.high_frequency_lrg() ) {

But I'm not sure if this change can be committed with this patch or if a new patch can be committed for it.

TobiHartmann · 2022-12-19T06:45:26Z

Tests passed and performance results looks good. I think the other change that you proposed should not be part of this.

openjdk · 2022-12-19T06:48:16Z

⚠️ @sunny868 the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout 8298813
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

openjdk · 2022-12-19T06:48:19Z

@sunny868 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:

8298813: [C2] Converting double to float cause a loss of precision and resulting crypto.aes scores fluctuate

Reviewed-by: 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 1626 new commits pushed to the master branch:

5e678f7: 8298824: C2 crash: assert(is_Bool()) failed: invalid node class: ConI
ba942c2: 8298244: AArch64: Optimize vector implementation of AddReduction for floating point
7938f8c: 8298639: Perform I/O operations in bulk for RandomAccessFile
bfa921a: 8160404: RelocationHolder constructors have bugs
bf9a8ce: 8249826: 5 javax/net/ssl/SSLEngine tests use @ignore w/o bug-id
0eeaeb8: 8298808: Check script code on detecting the base locales
81e23ab: 8298809: Clean up vm/compiler/InterfaceCalls JMH
3696711: Merge
f771c56: 8298797: Specification of some restricted methods is incorrect
0ba4734: 8287699: jdk/jfr/api/consumer/TestRecordingFileWrite.java fails with exception: java.lang.Exception: Found event that should not be there.
... and 1616 more: https://git.openjdk.org/jdk/compare/f85411f24a0cf55f8718aadd2297d95feb95564b...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TobiHartmann) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

…d resulting crypto.aes scores fluctuate

sunny868 · 2022-12-19T07:52:16Z

/integrate

openjdk · 2022-12-19T07:53:39Z

@sunny868
Your change (at version c2bf1d7) is now ready to be sponsored by a Committer.

TobiHartmann · 2022-12-19T07:55:40Z

/sponsor

openjdk · 2022-12-19T07:56:44Z

Going to push as commit 3637660.
Since your change was applied there have been 1626 commits pushed to the master branch:

5e678f7: 8298824: C2 crash: assert(is_Bool()) failed: invalid node class: ConI
ba942c2: 8298244: AArch64: Optimize vector implementation of AddReduction for floating point
7938f8c: 8298639: Perform I/O operations in bulk for RandomAccessFile
bfa921a: 8160404: RelocationHolder constructors have bugs
bf9a8ce: 8249826: 5 javax/net/ssl/SSLEngine tests use @ignore w/o bug-id
0eeaeb8: 8298808: Check script code on detecting the base locales
81e23ab: 8298809: Clean up vm/compiler/InterfaceCalls JMH
3696711: Merge
f771c56: 8298797: Specification of some restricted methods is incorrect
0ba4734: 8287699: jdk/jfr/api/consumer/TestRecordingFileWrite.java fails with exception: java.lang.Exception: Found event that should not be there.
... and 1616 more: https://git.openjdk.org/jdk/compare/f85411f24a0cf55f8718aadd2297d95feb95564b...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-12-19T07:57:03Z

@TobiHartmann @sunny868 Pushed as commit 3637660.

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

sunny868 · 2022-12-20T01:33:35Z

@TobiHartmann Thank you for your review. I have one more question for you, How did you test SPECjvm2008 performance? take the maximum or average value of multiple test results?

TobiHartmann · 2022-12-23T08:29:24Z

@sunny868 We take the average value of multiple test runs.

sunny868 · 2022-12-23T09:27:55Z

@sunny868 We take the average value of multiple test runs.

How many times do I need to run SPECjvm2008? And is this average from all jvm2008 benchmarks or a single one?
Are there any documentation or websites about performance testing for my reference?

TobiHartmann · 2022-12-23T09:57:55Z

We run the SPECjvm2008 benchmarks individually and gather different results (average, low/high, statistical significance, ...). I think the exact configuration differs depending on the platform and benchmark. Since this is part of our internal performance testing infrastructure, no public documentation is available.

8298813: [C2] Converting double to float cause a loss of precision an…

f017e49

…d resulting crypto.aes scores fluctuate

openjdk bot added the rfr Pull request is ready for review label Dec 15, 2022

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Dec 15, 2022

TobiHartmann reviewed Dec 16, 2022

View reviewed changes

TobiHartmann approved these changes Dec 19, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 19, 2022

8298813: [C2] Converting double to float cause a loss of precision an…

c2bf1d7

…d resulting crypto.aes scores fluctuate

openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 19, 2022

openjdk bot added the integrated Pull request has been integrated label Dec 19, 2022

openjdk bot closed this Dec 19, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Dec 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8298813: [C2] Converting double to float cause a loss of precision and resulting crypto.aes scores fluctuate #11685

JDK-8298813: [C2] Converting double to float cause a loss of precision and resulting crypto.aes scores fluctuate #11685

sunny868 commented Dec 15, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 15, 2022

openjdk bot commented Dec 15, 2022

mlbridge bot commented Dec 15, 2022 •

edited

Loading

sunny868 commented Dec 15, 2022

TobiHartmann left a comment

TobiHartmann Dec 16, 2022

sunny868 commented Dec 17, 2022

sunny868 commented Dec 17, 2022

sunny868 commented Dec 17, 2022

TobiHartmann commented Dec 19, 2022

openjdk bot commented Dec 19, 2022

openjdk bot commented Dec 19, 2022 •

edited

Loading

sunny868 commented Dec 19, 2022

openjdk bot commented Dec 19, 2022

TobiHartmann commented Dec 19, 2022

openjdk bot commented Dec 19, 2022

openjdk bot commented Dec 19, 2022

sunny868 commented Dec 20, 2022

TobiHartmann commented Dec 23, 2022

sunny868 commented Dec 23, 2022 •

edited

Loading

TobiHartmann commented Dec 23, 2022

	double _high_frequency_lrg; // Frequency at which LRG will be spilled for debug info
	double _high_frequency_lrg; // Frequency at which LRG will be spilled for debug info

JDK-8298813: [C2] Converting double to float cause a loss of precision and resulting crypto.aes scores fluctuate #11685

JDK-8298813: [C2] Converting double to float cause a loss of precision and resulting crypto.aes scores fluctuate #11685

Conversation

sunny868 commented Dec 15, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 15, 2022

openjdk bot commented Dec 15, 2022

mlbridge bot commented Dec 15, 2022 • edited Loading

Webrevs

sunny868 commented Dec 15, 2022

TobiHartmann left a comment

Choose a reason for hiding this comment

TobiHartmann Dec 16, 2022

Choose a reason for hiding this comment

sunny868 commented Dec 17, 2022

sunny868 commented Dec 17, 2022

sunny868 commented Dec 17, 2022

TobiHartmann commented Dec 19, 2022

openjdk bot commented Dec 19, 2022

openjdk bot commented Dec 19, 2022 • edited Loading

sunny868 commented Dec 19, 2022

openjdk bot commented Dec 19, 2022

TobiHartmann commented Dec 19, 2022

openjdk bot commented Dec 19, 2022

openjdk bot commented Dec 19, 2022

sunny868 commented Dec 20, 2022

TobiHartmann commented Dec 23, 2022

sunny868 commented Dec 23, 2022 • edited Loading

TobiHartmann commented Dec 23, 2022

sunny868 commented Dec 15, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 15, 2022 •

edited

Loading

openjdk bot commented Dec 19, 2022 •

edited

Loading

sunny868 commented Dec 23, 2022 •

edited

Loading