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

8266267: Remove unnecessary jumps in Intel Math Library StubRoutines #3784

Closed
wants to merge 1 commit into from

Conversation

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Apr 29, 2021

Hi all,

May I get reviews for this small change?

If I look at the StubRoutines::libmPow carefully, an unnecessary jump instruction can be found, which is generated here [1].

- - - [BEGIN] - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
StubRoutines::libmPow [0x00007f0f8455ddd1, 0x00007f0f8455ef05] (4404 bytes)
--------------------------------------------------------------------------------
  0x00007f0f8455ddd1:   push   %rbp
  0x00007f0f8455ddd2:   mov    %rsp,%rbp
  0x00007f0f8455ddd5:   jmpq   0x00007f0f8455ddda          <--- unnecessary jump
  0x00007f0f8455ddda:   sub    $0x28,%rsp
  0x00007f0f8455ddde:   vmovsd %xmm0,0x8(%rsp)
  0x00007f0f8455dde4:   vmovsd %xmm1,0x10(%rsp)
  ...

And similar issues can be found for StubRoutines::libmLog and StubRoutines::libmExp.

- - - [BEGIN] - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
StubRoutines::libmLog [0x00007f3fb94d48a6, 0x00007f3fb94d4b22] (636 bytes)
--------------------------------------------------------------------------------
  0x00007f3fb94d48a6:   push   %rbp
  0x00007f3fb94d48a7:   mov    %rsp,%rbp
  0x00007f3fb94d48aa:   jmpq   0x00007f3fb94d48af          <--- unnecessary jump
  0x00007f3fb94d48af:   sub    $0x18,%rsp
  0x00007f3fb94d48b3:   vmovsd %xmm0,(%rsp)
  0x00007f3fb94d48b8:   movabs $0x3ff0000000000000,%rax
  ...
- - - [BEGIN] - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
StubRoutines::libmExp [0x00007f3fb94d4579, 0x00007f3fb94d48a6] (813 bytes)
--------------------------------------------------------------------------------
  0x00007f3fb94d4579:   push   %rbp
  0x00007f3fb94d457a:   mov    %rsp,%rbp
  0x00007f3fb94d457d:   jmpq   0x00007f3fb94d4582          <--- unnecessary jump
  0x00007f3fb94d4582:   sub    $0x18,%rsp
  0x00007f3fb94d4586:   vmovsd %xmm0,0x8(%rsp)
  0x00007f3fb94d458c:   vunpcklpd %xmm0,%xmm0,%xmm0

I can't find a reason why we need an extra jump there.
So I suggest removing them.

Testing:

  • tier1 ~ tier3 on Linux/x64, no regression

Am I missing something?

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-8266267: Remove unnecessary jumps in Intel Math Library StubRoutines

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3784

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

Using diff file

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

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Apr 29, 2021

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

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 29, 2021

👋 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 29, 2021

@DamonFool
The hotspot-compiler label was successfully added.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 29, 2021

@DamonFool The hotspot-compiler label was already applied.

Loading

@DamonFool DamonFool marked this pull request as ready for review Apr 29, 2021
@openjdk openjdk bot added the rfr label Apr 29, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 29, 2021

Webrevs

Loading

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Looks good to me.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 29, 2021

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

8266267: Remove unnecessary jumps in Intel Math Library StubRoutines

Reviewed-by: thartmann, neliasso, kvn

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

  • 49d0458: 8266288: assert root method not found in witnessed_reabstraction_in_supers is too strong
  • 01415f3: 8266250: WebSocketTest and WebSocketProxyTest call assertEquals(List<byte[]>, List<byte[]>)
  • 5f15666: 8266078: Reader.read(CharBuffer) advances Reader position for read-only Charbuffers
  • 2a03739: 8266014: Regression brought by optimization done with JDK-4926314
  • 6bb71d9: 8264762: ByteBuffer.byteOrder(BIG_ENDIAN).asXBuffer.put(Xarray) and ByteBuffer.byteOrder(nativeOrder()).asXBuffer.put(Xarray) are slow
  • f0f6b0d: 8266027: The diamond finder does not find diamond candidates in field initializers
  • 8072ea5: 8238173: jshell - switch statement with a single default not return cause syntax error
  • c76ce28: 8265842: G1: Introduce API to run multiple separate tasks in a single gangtask
  • 294347b: 8265918: java/io/Console/CharsetTest.java failed with "expect: spawn id exp6 not open"
  • 84b52db: 8265666: Enable AIX build platform to make external debug symbols
  • ... and 12 more: https://git.openjdk.java.net/jdk/compare/69155f43c18839fec03d315ec6e4ba7b0797da30...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.

Loading

@openjdk openjdk bot added the ready label Apr 29, 2021
Copy link
Contributor

@neliasso neliasso left a comment

Looks good.

Loading

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Apr 29, 2021

Thanks @TobiHartmann and @neliasso .

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

How you found them?

Loading

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Apr 29, 2021

How you found them?

To make sure the x64's stub code works for x=-0.0 while doing #3712 , I took a look at the generated code.
Then I found the unnecessary jump at the beginning of dpow's stub.
Thanks.

Loading

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Apr 30, 2021

/integrate

Loading

@openjdk openjdk bot closed this Apr 30, 2021
@openjdk openjdk bot added integrated and removed ready labels Apr 30, 2021
@DamonFool DamonFool deleted the JDK-8266267 branch Apr 30, 2021
@openjdk openjdk bot removed the rfr label Apr 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 30, 2021

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

  • 2c381e0: 8262376: ReplaceCriticalClassesForSubgraphs.java fails if --with-build-jdk is used
  • 5ecef01: 8266217: ZGC: Improve the -Xlog:gc+init output for NUMA
  • 5d8c1cc: 8255410: Add ChaCha20 and Poly1305 support to SunPKCS11 provider
  • 46b4a14: 8266315: Problem list failing test java/awt/font/TextLayout/LigatureCaretTest.java
  • 42af7da: 8265933: Move Java monitor related fields from class Thread to JavaThread
  • 1afbab6: 8263998: Remove mentions of mc region in comments
  • 51b2fb5: 8266299: ProblemList runtime/stringtable/StringTableCleaningTest.java on linux-aarch64 with ZGC
  • 49d0458: 8266288: assert root method not found in witnessed_reabstraction_in_supers is too strong
  • 01415f3: 8266250: WebSocketTest and WebSocketProxyTest call assertEquals(List<byte[]>, List<byte[]>)
  • 5f15666: 8266078: Reader.read(CharBuffer) advances Reader position for read-only Charbuffers
  • ... and 19 more: https://git.openjdk.java.net/jdk/compare/69155f43c18839fec03d315ec6e4ba7b0797da30...master

Your commit was automatically rebased without conflicts.

Pushed as commit 51b2188.

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants