8277417: C1 LIR instruction for load-klass #6464

rkennke · 2021-11-18T20:16:27Z

In C1, the load of a Klass* out of an object is currently identified by a load of type T_ADDRESS with offset oopDest::klass_offset_in_bytes(). When encountering such load, this may be decoded when +CompressedClassPointers. This is problematic and ugly: if we ever emit a T_ADDRESS load with offset 8 or 4 (== klass_offset_in_bytes) that is not a Klass*, we would attempt to decode the result. We have been lucky so far.

Also, in Lilliput, we want to do something entirely different there, and need to be able to emit more complex code, possibly including runtime call.

The change introduces a new C1 LIR opcode OpLoadKlass, and refactors the implementations in c1_LIRAssembler_xyz.cpp to emit the code there, instead of mem2reg(). Notice that I could not test anything but x86, all other platforms only received very basic testing via GHA. It would be nice if respective maintainers could give it a try.

Testing:

tier1 (x86_64)
tier2 (x86_64)
tier3 (x86_64)

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8277417: C1 LIR instruction for load-klass

Reviewers

Igor Veresov (@veresov - Reviewer) ⚠️ Review applies to 988036f
Martin Doerr (@TheRealMDoerr - Reviewer)
Nick Gasson (@nick-arm - Reviewer)
Andrew Haley (@theRealAph - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6464

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

Using diff file

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

bridgekeeper · 2021-11-18T20:16:40Z

👋 Welcome back rkennke! 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 · 2021-11-18T20:18:32Z

@rkennke 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.

veresov

Nice! Thank you!

openjdk · 2021-11-18T23:27:18Z

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

8277417: C1 LIR instruction for load-klass

Reviewed-by: iveresov, mdoerr, ngasson, aph

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

d427c79: 8277428: G1: Move and inline G1STWIsAliveClosure::do_object_b
32839ba: 8266593: vmTestbase/nsk/jvmti/PopFrame/popframe011 fails with "assert(java_thread == _state->get_thread()) failed: Must be"
8051041: 8277534: Remove unused ReferenceProcessor::has_discovered_references
3f847fe: 8277385: Zero: Enable CompactStrings support
ca31ed5: 8275448: [REDO] AArch64: Implement string_compare intrinsic in SVE
4ff4301: 8224922: Access JavaFileObject from Element(s)
0a9e76c: 8277485: Zero: Fix fast{i,f}access_0 bytecodes handling
1c215f3: 8272773: Configurable card table card size
1d7cef3: 8276662: Scalability bottleneck in SymbolTable::lookup_common()
c79a485: 8277494: [BACKOUT] JDK-8276150 Quarantined jpackage apps are labeled as "damaged"
... and 66 more: https://git.openjdk.java.net/jdk/compare/b0a463fa59a1c3c554f48267525729bf89a2c5be...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.

mlbridge · 2021-11-19T14:43:59Z

Webrevs

01: Full - Incremental (3454c1bf)
00: Full (988036fa)

TheRealMDoerr

Looks like a nice change. I only found one problem on Power.

TheRealMDoerr · 2021-11-19T17:29:46Z

src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp

+  CodeEmitInfo* info = op->info();
+  if (info != NULL) {
+    add_debug_info_for_null_check_here(info);
+  }


I think this is incorrect for AIX. Note that the first page is not read protected on that OS. To make it consistent with other places, I suggest:

diff --git a/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp b/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp index a772e48f3be..23e03cb36e3 100644 --- a/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp +++ b/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp @@ -2733,7 +2733,11 @@ void LIR_Assembler::emit_load_klass(LIR_OpLoadKlass* op) { CodeEmitInfo* info = op->info(); if (info != NULL) { - add_debug_info_for_null_check_here(info); + if (!os::zero_page_read_protected() || !ImplicitNullChecks) { + explicit_null_check(obj, info); + } else { + add_debug_info_for_null_check_here(info); + } } if (UseCompressedClassPointers) {

Thank you! I pushed a fix for that.

Unfortunately, we have the info != NULL check twice, now. Otherwise, good.

nick-arm

I tested tier1 on 32-bit Arm and AArch64. 32-bit Arm had some failures but they don't seem to be related to this patch.

nick-arm · 2021-11-22T10:39:26Z

src/hotspot/cpu/arm/c1_LIRAssembler_arm.cpp

+    add_debug_info_for_null_check_here(info);
+  }
+
+  if (UseCompressedClassPointers) { // On 32 bit arm??


It's probably leftover from when the "arm" port supported both 32- and 64-bit.

theRealAph · 2021-11-22T10:53:44Z

src/hotspot/cpu/aarch64/c1_LIRAssembler_aarch64.cpp

@@ -986,14 +986,7 @@ void LIR_Assembler::mem2reg(LIR_Opr src, LIR_Opr dest, BasicType type, LIR_Patch
      __ ldr(dest->as_register(), as_Address(from_addr));
      break;
    case T_ADDRESS:
-      // FIXME: OMG this is a horrible kludge.  Any offset from an
-      // address that matches klass_offset_in_bytes() will be loaded
-      // as a word, not a long.


Ha! I am so glad to see this horrible kludge removed.

theRealAph

Thanks, a very welcome fix. I wish I had done something like this at the time of the AArch64 port, but I was neither brave enough nor knew enough

TheRealMDoerr

Nice change! Please remove the duplicated info != NULL check before integrating.

TheRealMDoerr · 2021-11-22T12:41:18Z

src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp

+  CodeEmitInfo* info = op->info();
+  if (info != NULL) {
+    add_debug_info_for_null_check_here(info);
+  }


Unfortunately, we have the info != NULL check twice, now. Otherwise, good.

rkennke · 2021-11-26T09:45:20Z

/integrate

openjdk · 2021-11-26T09:46:19Z

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

00a6238: 8277508: need to check has_predicated_vectors before calling scalable_predicate_reg_slots
3383c0d: 8277659: [TESTBUG] Microbenchmark ThreadOnSpinWaitProducerConsumer.java hangs
7e54d06: 8277165: jdeps --multi-release --print-module-deps fails if module-info.class in different versioned directories
eb4d886: 8277504: Use String.stripTrailing instead of hand-crafted method in SwingUtilities2
9879920: 8277825: Remove unused ReferenceProcessorPhaseTimes::_sub_phases_total_time_ms
f788834: 8277786: G1: Rename log2_card_region_per_heap_region used in G1CardSet
3034ae8: 8277631: ZGC: CriticalMetaspaceAllocation asserts
f0136ec: 8275687: runtime/CommandLine/PrintTouchedMethods test shouldn't catch RuntimeException
21e302a: 8270435: UT: MonitorUsedDeflationThresholdTest failed: did not find too_many string in output
a81e4fc: 8258117: jar tool sets the time stamp of module-info.class entries to the current time
... and 122 more: https://git.openjdk.java.net/jdk/compare/b0a463fa59a1c3c554f48267525729bf89a2c5be...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-11-26T09:46:35Z

@rkennke Pushed as commit 99e4bda.

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

8277417: C1 LIR instruction for load-klass

46eb0ad

openjdk bot added the hotspot hotspot-dev@openjdk.org label Nov 18, 2021

Add debug info for null checks

988036f

veresov approved these changes Nov 18, 2021

View changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 18, 2021

rkennke marked this pull request as ready for review November 19, 2021 14:37

openjdk bot added the rfr Pull request is ready for review label Nov 19, 2021

TheRealMDoerr suggested changes Nov 19, 2021

View changes

Fix null-check on PPC

3454c1b

nick-arm approved these changes Nov 22, 2021

View changes

theRealAph reviewed Nov 22, 2021

View changes

theRealAph approved these changes Nov 22, 2021

View changes

TheRealMDoerr approved these changes Nov 22, 2021

View changes

openjdk bot closed this Nov 26, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 26, 2021

8277417: C1 LIR instruction for load-klass #6464

8277417: C1 LIR instruction for load-klass #6464

rkennke commented Nov 18, 2021 •

edited by openjdk bot

bridgekeeper bot commented Nov 18, 2021

openjdk bot commented Nov 18, 2021

veresov left a comment •

edited

openjdk bot commented Nov 18, 2021 •

edited

mlbridge bot commented Nov 19, 2021 •

edited

TheRealMDoerr left a comment

TheRealMDoerr Nov 19, 2021

rkennke Nov 19, 2021

TheRealMDoerr Nov 22, 2021

nick-arm left a comment

nick-arm Nov 22, 2021

theRealAph Nov 22, 2021

theRealAph left a comment

TheRealMDoerr left a comment

TheRealMDoerr Nov 22, 2021

rkennke commented Nov 26, 2021

openjdk bot commented Nov 26, 2021

openjdk bot commented Nov 26, 2021

8277417: C1 LIR instruction for load-klass #6464

8277417: C1 LIR instruction for load-klass #6464

Conversation

rkennke commented Nov 18, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 18, 2021

openjdk bot commented Nov 18, 2021

veresov left a comment • edited

Choose a reason for hiding this comment

openjdk bot commented Nov 18, 2021 • edited

mlbridge bot commented Nov 19, 2021 • edited

Webrevs

TheRealMDoerr left a comment

Choose a reason for hiding this comment

TheRealMDoerr Nov 19, 2021

Choose a reason for hiding this comment

rkennke Nov 19, 2021

Choose a reason for hiding this comment

TheRealMDoerr Nov 22, 2021

Choose a reason for hiding this comment

nick-arm left a comment

Choose a reason for hiding this comment

nick-arm Nov 22, 2021

Choose a reason for hiding this comment

theRealAph Nov 22, 2021

Choose a reason for hiding this comment

theRealAph left a comment

Choose a reason for hiding this comment

TheRealMDoerr left a comment

Choose a reason for hiding this comment

TheRealMDoerr Nov 22, 2021

Choose a reason for hiding this comment

rkennke commented Nov 26, 2021

openjdk bot commented Nov 26, 2021

openjdk bot commented Nov 26, 2021

rkennke commented Nov 18, 2021 •

edited by openjdk bot

veresov left a comment •

edited

openjdk bot commented Nov 18, 2021 •

edited

mlbridge bot commented Nov 19, 2021 •

edited