8283699: Improve the peephole mechanism of hotspot #8025

merykitty · 2022-03-29T23:58:39Z

Hi,

The current peephole mechanism has several drawbacks:

Can only match and remove adjacent instructions.
Cannot match machine ideal nodes (e.g MachSpillCopyNode).
Can only replace 1 instruction, the position of insertion is limited to the position at which the matched nodes reside.
Is actually broken since the nodes are not connected properly and OptoScheduling requires true dependencies between nodes.

The patch proposes to enhance the peephole mechanism by allowing a peep rule to call into a dedicated function, which takes the responsibility to perform all required transformations on the basic block. This allows the peephole mechanism to perform several transformations effectively in a more fine-grain manner.

The patch uses the peephole optimisation to perform some classic peepholes, transforming on x86 the sequences:

mov r1, r2    ->    lea r1, [r2 + r3/i]
add r1, r3/i

and

mov r1, r2    ->    lea r1, [r2 << i], with i = 1, 2, 3
shl r1, i

On the added benchmarks, the transformations show positive results:

Benchmark             Mode  Cnt     Score     Error  Units
LeaPeephole.B_D_int   avgt    5  1200.490 ± 104.662  ns/op
LeaPeephole.B_D_long  avgt    5  1211.439 ±  30.196  ns/op
LeaPeephole.B_I_int   avgt    5  1118.831 ±   7.995  ns/op
LeaPeephole.B_I_long  avgt    5  1112.389 ±  15.838  ns/op
LeaPeephole.I_S_int   avgt    5  1262.528 ±   7.293  ns/op
LeaPeephole.I_S_long  avgt    5  1223.820 ±  17.777  ns/op

Benchmark             Mode  Cnt    Score    Error  Units
LeaPeephole.B_D_int   avgt    5  860.889 ±  6.089  ns/op
LeaPeephole.B_D_long  avgt    5  945.455 ± 21.603  ns/op
LeaPeephole.B_I_int   avgt    5  849.109 ±  9.809  ns/op
LeaPeephole.B_I_long  avgt    5  851.283 ± 16.921  ns/op
LeaPeephole.I_S_int   avgt    5  976.594 ± 23.004  ns/op
LeaPeephole.I_S_long  avgt    5  936.984 ±  9.601  ns/op

A following patch would add IR tests for these transformations since the IR framework has not been able to parse the ideal scheduling yet although printing the scheduling itself has been made possible recently.

Thank you very much.

Progress

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

Issue

JDK-8283699: Improve the peephole mechanism of hotspot

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer)
Dean Long (@dean-long - Reviewer) ⚠️ Review applies to 566b8dd1

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8025

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

Using diff file

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

bridgekeeper · 2022-03-29T23:59:19Z

👋 Welcome back merykitty! 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-03-30T00:01:58Z

@merykitty 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-04-01T19:27:59Z

Webrevs

merykitty · 2022-04-06T08:06:53Z

In case this has not reached the mailing list, may someone take a look at this, please.
Thank you very much.

bridgekeeper · 2022-05-04T12:41:36Z

@merykitty This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

dean-long · 2022-10-06T08:39:45Z

src/hotspot/cpu/x86/peephole_x86.cpp

+
+#ifdef COMPILER2
+
+#include "opto/peephole.hpp"


I don't see why opto/peephole.hpp is useful. Why not just include peephole_x86.hpp? Then the empty peephole_.hpp for the other platforms are no longer needed.

opto/peephole.hpp is needed from the generated ad_x86_peephole.cpp so that addI_rRegNode::peephole can call the helper functions.

How about including peephole_<cpu>.hpp only when "peepprocedure" is seen, and delete opto/peephole.hpp and empty peephole_<cpu>.hpp files?

What do you think if I include peephole_x86.hpp in x86_64.ad in a source hpp block? This will result in the include appearing in ad_x86.hpp, which will be transitively included in ad_x86_peephole.cpp. Thanks a lot.

Yes, good idea.

Considering peephole_x86.cpp contains only x64-specific code, I had a suggestion to rename it into peephole_x86_64.cpp and move #ifdef _LP64 into peephole_x86.hpp to guard x64-specific declarations. But if you intend to include the header directly from x86_64.ad, you can rename the header to peephole_x86_64.hpp and get rid of #ifdef _LP64 completely.

I have refactored that and removed opto/peephole.hpp as well as other peephole headers. peephole_x86.cpp is also renamed to peephole_x86_64.cpp. Thanks very much.

src/hotspot/cpu/x86/peephole_x86.cpp

vnkozlov

I started build testing. But I can only verify 64-bit.
@merykitty Can you verify 32 build too?

iwanowww · 2022-10-10T23:03:05Z

src/hotspot/cpu/x86/x86_64.ad

 // definitions necessary in the rest of the architecture description
+
+source_hpp %{
+  #include CPU_HEADER(peephole)


Why don't you simply include peephole_x86_64.hpp here?

merykitty · 2022-10-11T00:23:23Z

@vnkozlov I saw GHA being happy with x86_32, I also tried cross-compiling locally for 32-bit build.

vnkozlov · 2022-10-11T02:05:05Z

@vnkozlov I saw GHA being happy with x86_32, I also tried cross-compiling locally for 32-bit build.

Good. My builds in tier1 also passed. I don't have any more comments.

merykitty · 2022-10-11T04:22:12Z

Thank you very much for your reviews!
/integrate

openjdk · 2022-10-11T04:22:42Z

@merykitty This pull request has not yet been marked as ready for integration.

merykitty · 2022-10-11T20:29:32Z

@vnkozlov Could you reapprove this PR, please?

vnkozlov

Good.

merykitty · 2022-10-11T23:04:52Z

Thanks a lot
/integrate

openjdk · 2022-10-11T23:05:57Z

@merykitty
Your change (at version 9a0b65f) is now ready to be sponsored by a Committer.

vnkozlov · 2022-10-12T00:29:00Z

/sponsor

openjdk · 2022-10-12T00:31:16Z

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

94a9b04: 8295013: OopStorage should derive from CHeapObjBase
3a980b9: 8295168: Remove superfluous period in @throws tag description
9bb932c: 8295154: Documentation for RemoteExecutionControl.invoke(Method) inherits non-existent documentation
945950d: 8295069: [PPC64] Performance regression after JDK-8290025
d362e16: 8294689: The SA transported_core.html file needs quite a bit of work
07946aa: 8289552: Make intrinsic conversions between bit representations of half precision values and floats
2586b1a: 8295155: Incorrect javadoc of java.base module
e1a77cf: 8295163: Remove old hsdis Makefile
3c7ae12: 8294821: Class load improvement for AES crypto engine
619cd82: 8294702: BufferedInputStream uses undefined value range for markpos
... and 177 more: https://git.openjdk.org/jdk/compare/3419363e89eaeef61a44fa1ab12d6a355323eb68...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-10-12T00:31:38Z

@vnkozlov @merykitty Pushed as commit 703a6ef.

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

merykitty added 18 commits March 6, 2022 22:42

draft

09eeb6b

draft2

a82e9cc

typos

16a09f7

running

2323329

docs, add shift

e954750

temporary

2b37f37

Merge branch 'master' into peephole

7a92b70

wip

fc2fee7

adjust phaseX, output

24b6164

prototype

6271a7d

names and docs

9819bf8

inconsistent override

fca1eaa

fix build

5063f3b

build fix

d7b1630

attempt

079a4ab

attempt

712efed

refactor

6806432

fix?

4744915

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 30, 2022

merykitty added 4 commits March 30, 2022 20:33

refactor

adf072f

Merge branch 'master' into peephole

0f49105

add benchmark

7f0d278

some fix

c63abbd

merykitty marked this pull request as ready for review April 1, 2022 19:22

openjdk bot added the rfr Pull request is ready for review label Apr 1, 2022

dean-long reviewed Oct 6, 2022

View reviewed changes

src/hotspot/cpu/x86/peephole_x86.cpp Show resolved Hide resolved

dean-long reviewed Oct 6, 2022

View reviewed changes

src/hotspot/cpu/x86/peephole_x86.cpp Show resolved Hide resolved

refactor includes

566b8dd

dean-long approved these changes Oct 10, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 10, 2022

vnkozlov reviewed Oct 10, 2022

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Oct 10, 2022

iwanowww reviewed Oct 10, 2022

View reviewed changes

rename header to x64 specific

72a9499

rename macro guard

9a0b65f

vnkozlov approved these changes Oct 11, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 11, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 11, 2022

openjdk bot added the integrated Pull request has been integrated label Oct 12, 2022

openjdk bot closed this Oct 12, 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 Oct 12, 2022

merykitty deleted the peephole branch April 27, 2023 03:01

8283699: Improve the peephole mechanism of hotspot #8025

8283699: Improve the peephole mechanism of hotspot #8025

Uh oh!

Conversation

merykitty commented Mar 29, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Mar 29, 2022

Uh oh!

openjdk bot commented Mar 30, 2022

Uh oh!

mlbridge bot commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

merykitty commented Apr 6, 2022

Uh oh!

bridgekeeper bot commented May 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merykitty commented Oct 11, 2022

Uh oh!

vnkozlov commented Oct 11, 2022

Uh oh!

merykitty commented Oct 11, 2022

Uh oh!

openjdk bot commented Oct 11, 2022

Uh oh!

merykitty commented Oct 11, 2022

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

merykitty commented Oct 11, 2022

Uh oh!

openjdk bot commented Oct 11, 2022

Uh oh!

vnkozlov commented Oct 12, 2022

Uh oh!

openjdk bot commented Oct 12, 2022

Uh oh!

openjdk bot commented Oct 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

merykitty commented Mar 29, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 1, 2022 •

edited

Loading