8363837: Make StubRoutines::crc_table_adr() into platform-specific method #26434

vnkozlov · 2025-07-22T21:56:13Z

StubRoutines::_crc_table_adr and _crc32c_table_addr are used by initial stubs. In Leyden these addresses should be recorded in AOTCodeAddressTable to be used by AOTed stubs. But recording in AOTCodeAddressTable is done in AOTCodeCache::init2() which is called before initial stubs are generated and _crc*_table_addr are set.

We need to move _crc_table_addr and _crc32c_table_addr initialization to preuniverse blob to be available in AOTCodeCache::init2().

I also renamed _crc_table_adr to _crc_table_addr to match other names.

During testing I found that -Xlog:stubs affects empty blobs generation. There was typo there: return nullptr; was in wrong place.

I have to specify sizes of preuniverse blobs so they are called after I fixed typo. 32 bytes is cache line size on most CPUs. Note, there will be no assembler code generation for this case: _crc*_table_addr are initialized by address of C tables.

I did not move _crc*_table_addr declaration in stubDeclarations.hpp from init to preuniverse blob. I tried but there is issue: they require to specify stub name and I can move updateBytesCRC32 stub declaration. I tries add fake stub but it did not work. I would prefer if I could use nullptr instead of stub in such case. But it will complicate other code. I think it is fine do_entry() macro only creates field and accessor function.

Tested: tier1-4,tier10-rt,xcomp,stress

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-8363837: Make StubRoutines::crc_table_adr() into platform-specific method (Bug - P4)

Reviewers

Andrew Dinn (@adinn - Reviewer)
Yudi Zheng (@mur47x111 - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26434

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…rse stubs

bridgekeeper · 2025-07-22T21:56:46Z

👋 Welcome back kvn! 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-07-22T21:57:29Z

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

8363837: Make StubRoutines::crc_table_adr() into platform-specific method

Reviewed-by: adinn, yzheng

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

9e209fe: 8364110: Remove unused methods in GCCause
75ff7e1: 8361712: Improve ShenandoahAsserts printing
06fdb61: 8361964: Remove outdated algorithms from requirements and add PBES2 algorithms
... and 35 more: https://git.openjdk.org/jdk/compare/ea6674fec8702eea481afa7ca7e522cbacd53841...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-07-22T21:58:11Z

@vnkozlov The following labels will be automatically applied to this pull request:

graal
hotspot

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

vnkozlov · 2025-07-22T21:59:09Z

@adinn please look

mlbridge · 2025-07-22T22:03:10Z

Webrevs

vnkozlov · 2025-07-22T22:32:33Z

and I can move updateBytesCRC32 stub declaration.

and I can NOT move updateBytesCRC32 stub declaration.

vnkozlov · 2025-07-23T01:07:58Z

@TheRealMDoerr @offamitkumar @RealFYang please test these changes on your platforms.

adinn · 2025-07-23T13:19:35Z

@vnkozlov I'll note straight off that the situation here is complicated by the fact that crc_table_adr and crc32c_table_addr are pseudo-'entries'. They do not actually hold (code or data) addresses in generated code. What they really identify is an arch-specific address for foreign (C++) data that is assigned as a side-effect of performing generation for other entries. the only reason for marking them as entries is so we get a generated field and associated getter in class StubRoutines.

I'm not sure we need to care too much about their pseudo-entry status. More importantly, I dislike your solution of moving the initialization into generate_preuniverse_stubs(). This moves that initialization step away from the generator code that uses the resulting value, making it harder to see how the intrinsic works. It also has the unfortunate side effect of forcing us to create a preuinverse blob for every arch even if when we don't really need one.

You are right that we cannot add these addresses to the external addresses list under AOTCodeCache::init2() (via the call to table->init_extrs()) because the assignment of the pseudo-entry has not yet happened. However, we have two alternatives.

We can simply pretend that they are real entries add them to the stub addresses list under AOTCodeCache::init_early_stubs_table() (via the call to table->init_early_stubs()). That will successfully advertise them after they have been created as addresses that need relocation at AOT Cache load. However, it will also misclassify them as addresses in generated code when they are actually foreign addresses. Do we care about that misclassification (do we use a different reloc for stubs addresses vs external addresses?)
We can leave the external addresses table open at the end of the call to table->init_extrs()`. When we enter table->init_early_stubs() we can add these two extra 'pseudo-stub' addresses to the external addresses list and close it before registering the early stubs addresses.

Both of these seem to me to be simpler and cleaner ways to deal with the address publication problem. What do you think?

vnkozlov · 2025-07-23T15:04:07Z

Thank you, @adinn for you suggestions.

do we use a different reloc for stubs addresses vs external addresses?

will not work. They could be different. Stubs use runtime_call relic and crc uses external_word.

I considered 2. when I worked on this but there is assumption that we have all external addresses recorded when we look for matching address in AOTCodeAddressTable::id_for_address(). There is specific assert there. And we have relocation in all stubs and adapters. I am concern about some concurrency issues if we don't satisfy the assumption.

An other solution would be to have separate StubRoutines::get_crc_table_addr() which can be use early by AOT code and later by generate_initial_stubs(). It will check local value (StubRoutines::x86::_crc32c_table on x86, for example) and calculate it if needed.

What do you think?

adinn · 2025-07-23T15:42:34Z

Another solution would be to have separate StubRoutines::get_crc_table_addr() which can be use early by AOT code and later by generate_initial_stubs(). It will check local value (StubRoutines::x86::_crc32c_table on x86, for example) and calculate it if needed.

Yes, I think it would be much better if we removed the entry declarations for crc_table_adr and crc32c_table_addr from stubDeclarations.hpp and instead always looked up the address using a getter method declared in the shared header and implemented in each arch.

We don't need a generated field in class StubRoutines nor any way to track the value as we do for real generated addresses. So, using the declaration mechanism was the wrong way to do this.

vnkozlov · 2025-07-23T21:26:06Z

Here is new version of fix as we discussed.

RealFYang · 2025-07-24T07:16:25Z

@TheRealMDoerr @offamitkumar @RealFYang please test these changes on your platforms.

Thanks for the ping. I performed tier1 test and I see the result is clean with the latest version on linux-riscv64.

adinn · 2025-07-24T07:42:03Z

src/hotspot/cpu/zero/stubDeclarations_zero.hpp

@@ -37,7 +37,7 @@
                                      do_arch_blob,                     \
                                      do_arch_entry,                    \
                                      do_arch_entry_init)               \
-  do_arch_blob(initial, 0)                                              \
+  do_arch_blob(initial, 32)                                             \


Do we need this any more?

Yes, to call generate_initial_stubs() where _call_stub_entry is set.

Ah, yes, how annoying.

adinn · 2025-07-24T07:48:40Z

src/hotspot/share/runtime/stubRoutines.hpp

@@ -304,6 +304,9 @@ class StubRoutines: AllStatic {
    return dest_uninitialized ? _arrayof_oop_disjoint_arraycopy_uninit : _arrayof_oop_disjoint_arraycopy;
  }



Could we have a comment here to note that 1) this method is implemented in architecture-specific code 2) any table that is returned must be allocated once-only in foreign memory rather generated in the code cache.

mur47x111 · 2025-07-24T13:23:26Z

src/hotspot/share/jvmci/vmStructs_jvmci.cpp

@@ -417,8 +417,6 @@
  static_field(StubRoutines,                _dilithiumMontMulByConstant,                      address)                               \
  static_field(StubRoutines,                _dilithiumDecomposePoly,                          address)                               \
  static_field(StubRoutines,                _updateBytesCRC32,                                address)                               \
-  static_field(StubRoutines,                _crc_table_addr,                                  address)                               \


Could you please export via CompilerToVM::Data?

diff --git a/src/hotspot/share/jvmci/jvmciCompilerToVM.hpp b/src/hotspot/share/jvmci/jvmciCompilerToVM.hpp index 41531b083fc..71331b578a5 100644 --- a/src/hotspot/share/jvmci/jvmciCompilerToVM.hpp +++ b/src/hotspot/share/jvmci/jvmciCompilerToVM.hpp @@ -131,6 +131,8 @@ class CompilerToVM { static address dlog10; static address dpow; + static address crc_table_addr; + static address symbol_init; static address symbol_clinit; diff --git a/src/hotspot/share/jvmci/jvmciCompilerToVMInit.cpp b/src/hotspot/share/jvmci/jvmciCompilerToVMInit.cpp index b6d919fdfe9..8a1a02d62b3 100644 --- a/src/hotspot/share/jvmci/jvmciCompilerToVMInit.cpp +++ b/src/hotspot/share/jvmci/jvmciCompilerToVMInit.cpp @@ -151,6 +151,8 @@ address CompilerToVM::Data::dlog; address CompilerToVM::Data::dlog10; address CompilerToVM::Data::dpow; +address CompilerToVM::Data::crc_table_addr; + address CompilerToVM::Data::symbol_init; address CompilerToVM::Data::symbol_clinit; @@ -289,6 +291,7 @@ void CompilerToVM::Data::initialize(JVMCI_TRAPS) { SET_TRIGFUNC_OR_NULL(dtanh); SET_TRIGFUNC_OR_NULL(dcbrt); + SET_TRIGFUNC_OR_NULL(crc_table_addr); #undef SET_TRIGFUNC_OR_NULL diff --git a/src/hotspot/share/jvmci/vmStructs_jvmci.cpp b/src/hotspot/share/jvmci/vmStructs_jvmci.cpp index 1408cb09b0a..88d098468e9 100644 --- a/src/hotspot/share/jvmci/vmStructs_jvmci.cpp +++ b/src/hotspot/share/jvmci/vmStructs_jvmci.cpp @@ -147,6 +147,7 @@ static_field(CompilerToVM::Data, dlog, address) \ static_field(CompilerToVM::Data, dlog10, address) \ static_field(CompilerToVM::Data, dpow, address) \ + static_field(CompilerToVM::Data, crc_table_addr, address) \ \ static_field(CompilerToVM::Data, symbol_init, address) \ static_field(CompilerToVM::Data, symbol_clinit, address) \

@mur47x111 sure. But do you need also crc32c_table_addr? And I don't see initialization. Can you prepare full patch for JVMCI which I can integrate?

Initialization is done by SET_TRIGFUNC_OR_NULL(). I see.

Okay, I will add crc32c_table_addr myself then.

I see that crc32c_table_addr() failed on Aarch64 because we should not call it.
I will go with your patch @mur47x111 then and let you solve crc32c.

Thanks! I also ran across the ShouldNotCallThis on aarch64 and decided to drop that because anyway we don't use crc32c_table_addr. I think crc32c_table_addr is only used internally in HotSpot but not via intrinsification call. Let's only export crc_table_addr for now. I will examine the crc32c_table_addr later

Thank you, @mur47x111

adinn

Looks ok to me.

vnkozlov · 2025-07-24T15:42:34Z

Thanks for the ping. I performed tier1 test and I see the result is clean with the latest version on linux-riscv64.

@RealFYang thank you for testing.

vnkozlov · 2025-07-24T15:49:02Z

I updated title to reflect current implementation.

vnkozlov · 2025-07-24T15:49:34Z

Thank you @adinn

mur47x111

LGTM

vnkozlov · 2025-07-25T16:45:23Z

Thank you all for reviews.

vnkozlov · 2025-07-25T16:45:29Z

/integrate

openjdk · 2025-07-25T16:47:13Z

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

9e209fe: 8364110: Remove unused methods in GCCause
75ff7e1: 8361712: Improve ShenandoahAsserts printing
06fdb61: 8361964: Remove outdated algorithms from requirements and add PBES2 algorithms
... and 35 more: https://git.openjdk.org/jdk/compare/ea6674fec8702eea481afa7ca7e522cbacd53841...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-07-25T16:47:22Z

@vnkozlov Pushed as commit 89fe586.

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

8363837: Move StubRoutines::_crc_table_adr initialization to preunive…

08f2728

…rse stubs

openjdk bot added the rfr Pull request is ready for review label Jul 22, 2025

openjdk bot added graal [email protected] hotspot [email protected] labels Jul 22, 2025

Fixed Zero VM

6459712

graalvmbot mentioned this pull request Jul 23, 2025

[JDK-8363915] Adapt JDK-8363837: Make StubRoutines::crc_table_adr() into platform-specific method oracle/graal#11749

Merged

Rework the fix

f0378d2

adinn reviewed Jul 24, 2025

View reviewed changes

mur47x111 reviewed Jul 24, 2025

View reviewed changes

vnkozlov added 2 commits July 24, 2025 08:13

Add comment

71d29f9

JVMCI fix

eaba165

adinn approved these changes Jul 24, 2025

View reviewed changes

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

openjdk bot removed the ready Pull request is ready to be integrated label Jul 24, 2025

vnkozlov changed the title ~~8363837: Move StubRoutines::_crc_table_adr initialization to preuniverse stubs~~ 8363837: Make StubRoutines::crc_table_adr() into platform-specific method Jul 24, 2025

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

mur47x111 approved these changes Jul 25, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jul 25, 2025

openjdk bot closed this Jul 25, 2025

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

vnkozlov deleted the 8363837 branch July 25, 2025 16:49

zapster mentioned this pull request Aug 1, 2025

[GR-68382] Update labsjdk to 26+9-jvmci-b01 oracle/graal#11851

Merged

		@@ -304,6 +304,9 @@ class StubRoutines: AllStatic {
		return dest_uninitialized ? _arrayof_oop_disjoint_arraycopy_uninit : _arrayof_oop_disjoint_arraycopy;
		}

8363837: Make StubRoutines::crc_table_adr() into platform-specific method #26434

8363837: Make StubRoutines::crc_table_adr() into platform-specific method #26434

Uh oh!

Conversation

vnkozlov commented Jul 22, 2025 • 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 Jul 22, 2025

Uh oh!

openjdk bot commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jul 22, 2025

Uh oh!

vnkozlov commented Jul 22, 2025

Uh oh!

mlbridge bot commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

vnkozlov commented Jul 22, 2025

Uh oh!

vnkozlov commented Jul 23, 2025

Uh oh!

adinn commented Jul 23, 2025

Uh oh!

vnkozlov commented Jul 23, 2025

Uh oh!

adinn commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vnkozlov commented Jul 23, 2025

Uh oh!

RealFYang commented Jul 24, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vnkozlov Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

adinn left a comment

Choose a reason for hiding this comment

Uh oh!

vnkozlov commented Jul 24, 2025

Uh oh!

vnkozlov commented Jul 24, 2025

Uh oh!

vnkozlov commented Jul 24, 2025

Uh oh!

mur47x111 left a comment

Choose a reason for hiding this comment

Uh oh!

vnkozlov commented Jul 25, 2025

vnkozlov commented Jul 22, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jul 22, 2025 •

edited

Loading

mlbridge bot commented Jul 22, 2025 •

edited

Loading

adinn commented Jul 23, 2025 •

edited

Loading

vnkozlov Jul 24, 2025 •

edited

Loading