8346890: AArch64: Type profile counters generate suboptimal code #23012

theRealAph · 2025-01-09T16:30:49Z

Type profile counters are emitted many times in C1-generated code. The generator was written a long time ago before we knew how best to write AArch64 code, and the generated code is rather suboptimal.

This PR reduces the size of a typical bimorphic type profile counter from 33 to 27 instructions.

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-8346890: AArch64: Type profile counters generate suboptimal code (Enhancement - P4)

Reviewers

Aleksey Shipilev (@shipilev - Reviewer)
Andrew Dinn (@adinn - Reviewer) Review applies to 0e84904f

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23012

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-01-09T16:31:49Z

👋 Welcome back aph! 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 · 2025-01-09T16:32:43Z

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

8346890: AArch64: Type profile counters generate suboptimal code

Reviewed-by: shade, adinn

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

4a375e5: 8344232: [PPC64] secondary_super_cache does not scale well: C1 and interpreter
0df9dcb: 8346572: Check is_reserved() before using ReservedSpace instances
a09f06d: 8348265: RMIConnectionImpl: Remove Subject.callAs on MarshalledObject
0395593: 8346751: Internal java compiler error with type annotations in constants expression in constant fields
2daafe4: 8348283: java.lang.classfile.components.snippets.PackageSnippets shipped in java.base.jmod
50ca450: 8340784: Remove PassFailJFrame constructor with screenshots
416d469: 8347008: beancontext package spec does not clearly explain why the API is deprecated
471d63c: 8343609: Broken links in java.xml
7f16a08: 8348240: Remove SystemDictionaryShared::lookup_super_for_unregistered_class()
48ece07: 8282862: AwtWindow::SetIconData leaks old icon handles if an exception is detected
... and 303 more: https://git.openjdk.org/jdk/compare/bd0fde71d9113bad902e71b61f0ca44c650809ef...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.

openjdk · 2025-01-09T16:33:15Z

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

hotspot

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 · 2025-01-09T16:37:07Z

Webrevs

shipilev

Can you explain a bit more here? I think I get why would we want to rewrite lea+ldr to slot_at.

I do not quite understand why do we rewrite this one:

-    Address data_addr(mdo, md->byte_offset_of_slot(data, ReceiverTypeData::receiver_count_offset(i)));
-    __ addptr(data_addr, DataLayout::counter_increment);
+    __ addptr(slot_at(ReceiverTypeData::receiver_count_offset(i)),
+              DataLayout::counter_increment);

Does it really optimize anything to rewrite it to slot_at? If so, shouldn't this one in the other hunk also get rewritten?

         Address data_addr(mdo, md->byte_offset_of_slot(data, VirtualCallData::receiver_count_offset(i)));
          __ addptr(data_addr, DataLayout::counter_increment);

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp

theRealAph · 2025-01-10T11:00:38Z

Can you explain a bit more here? I think I get why would we want to rewrite lea+ldr to slot_at.

I do not quite understand why do we rewrite this one:
-    Address data_addr(mdo, md->byte_offset_of_slot(data, ReceiverTypeData::receiver_count_offset(i)));
-    __ addptr(data_addr, DataLayout::counter_increment);
+    __ addptr(slot_at(ReceiverTypeData::receiver_count_offset(i)),
+              DataLayout::counter_increment);
Does it really optimize anything to rewrite it to slot_at? If so, shouldn't this one in the other hunk also get rewritten?

It's a safety in depth patch.
It's good practice always to check variable offsets in order to ensure that they are reachable. addptr has an embedded form_address, so this is not strictly necessary. It is, however, harmless.

         Address data_addr(mdo, md->byte_offset_of_slot(data, VirtualCallData::receiver_count_offset(i)));
          __ addptr(data_addr, DataLayout::counter_increment);

Yes, it should.

theRealAph · 2025-01-10T12:42:30Z

For clarity: every variable-sized load/store offset generated by C1 should be range-checked, and when it is out of range the load/store should be split. We already do this in C2. I'll wrap up any missing examples in a later patch.

adinn

Changes look good apart from a nit over the comment

adinn · 2025-01-23T11:26:42Z

src/hotspot/cpu/aarch64/macroAssembler_aarch64.hpp

@@ -1173,6 +1173,7 @@ class MacroAssembler: public Assembler {

  // Arithmetics

+  // Clobber: rscratch2


I'm not entirely sure why this has been added as part of this fix. It's ok as a comment but 1) I'm not sure why you do not also note that addptr also clobbers rscratch1 and 2) cmpptr clobbers rscratch1 but not rscratch2 but it is not clear that this comment only applies to addptr. Perhaps a newline between the two methods would make that clearer.

Fixed.
As to why the comment change is part of this particular fix, I always put a little flag over a pothole when I see one.

theRealAph · 2025-01-24T10:38:05Z

/integrate

openjdk · 2025-01-24T10:38:45Z

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

4a375e5: 8344232: [PPC64] secondary_super_cache does not scale well: C1 and interpreter
0df9dcb: 8346572: Check is_reserved() before using ReservedSpace instances
a09f06d: 8348265: RMIConnectionImpl: Remove Subject.callAs on MarshalledObject
0395593: 8346751: Internal java compiler error with type annotations in constants expression in constant fields
2daafe4: 8348283: java.lang.classfile.components.snippets.PackageSnippets shipped in java.base.jmod
50ca450: 8340784: Remove PassFailJFrame constructor with screenshots
416d469: 8347008: beancontext package spec does not clearly explain why the API is deprecated
471d63c: 8343609: Broken links in java.xml
7f16a08: 8348240: Remove SystemDictionaryShared::lookup_super_for_unregistered_class()
48ece07: 8282862: AwtWindow::SetIconData leaks old icon handles if an exception is detected
... and 303 more: https://git.openjdk.org/jdk/compare/bd0fde71d9113bad902e71b61f0ca44c650809ef...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-01-24T10:39:07Z

@theRealAph Pushed as commit 5a0bdd0.

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

Andrew Haley added 2 commits January 1, 2025 22:03

tmp

c952a34

8346890: AArch64: Profile counters generate suboptimal code

3b93519

openjdk bot added the rfr Pull request is ready for review label Jan 9, 2025

openjdk bot added the hotspot hotspot-dev@openjdk.org label Jan 9, 2025

shipilev reviewed Jan 10, 2025

View reviewed changes

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp Outdated Show resolved Hide resolved

Leave for later

0e84904

adinn approved these changes Jan 23, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 23, 2025

Fix comments

fc1c714

openjdk bot removed the ready Pull request is ready to be integrated label Jan 23, 2025

shipilev approved these changes Jan 24, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 24, 2025

openjdk bot added the integrated Pull request has been integrated label Jan 24, 2025

openjdk bot closed this Jan 24, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8346890: AArch64: Type profile counters generate suboptimal code #23012

8346890: AArch64: Type profile counters generate suboptimal code #23012

theRealAph commented Jan 9, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 9, 2025

openjdk bot commented Jan 9, 2025 •

edited

Loading

openjdk bot commented Jan 9, 2025

mlbridge bot commented Jan 9, 2025 •

edited

Loading

shipilev left a comment •

edited

Loading

theRealAph commented Jan 10, 2025

theRealAph commented Jan 10, 2025

adinn left a comment

adinn Jan 23, 2025 •

edited

Loading

theRealAph Jan 23, 2025

theRealAph commented Jan 24, 2025

openjdk bot commented Jan 24, 2025

openjdk bot commented Jan 24, 2025

		@@ -1173,6 +1173,7 @@ class MacroAssembler: public Assembler {

		// Arithmetics

		// Clobber: rscratch2

8346890: AArch64: Type profile counters generate suboptimal code #23012

8346890: AArch64: Type profile counters generate suboptimal code #23012

Conversation

theRealAph commented Jan 9, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 9, 2025

openjdk bot commented Jan 9, 2025 • edited Loading

openjdk bot commented Jan 9, 2025

mlbridge bot commented Jan 9, 2025 • edited Loading

Webrevs

shipilev left a comment • edited Loading

Choose a reason for hiding this comment

theRealAph commented Jan 10, 2025

theRealAph commented Jan 10, 2025

adinn left a comment

Choose a reason for hiding this comment

adinn Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

theRealAph Jan 23, 2025

Choose a reason for hiding this comment

theRealAph commented Jan 24, 2025

openjdk bot commented Jan 24, 2025

openjdk bot commented Jan 24, 2025

theRealAph commented Jan 9, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jan 9, 2025 •

edited

Loading

mlbridge bot commented Jan 9, 2025 •

edited

Loading

shipilev left a comment •

edited

Loading

adinn Jan 23, 2025 •

edited

Loading