On Fri, 11 Apr 2025 09:09:05 GMT, Kim Barrett <kbarr...@openjdk.org> wrote:
>> On a second thought, I think we should do this shift before this PR, so that
>> it is cleanly backportable. This bug messes with concurrent GC invariants,
>> so it would be nice to fix it in current LTSes.
>
> As far as I can tell, intrinsic selection only applies when the call target is
> exactly the intrinsically attributed method. (Possibly after optimizations
> that lead to a call to that specific method.) And that's obviously necessary
> for correctness of applying intrinsics.
>
> The documentation for `@IntrinsicCandidate` suggests it is "better to make
> intrinsics be static methods", but non-static methods are certainly permitted,
> and I've found several. In particular, the method in question,
> `Reference::get`, has been a virtual intrinsic with at least three
> (non-intrinsic) overrides (SoftReference, PhantomReference, and
> FinalReference) for a long time.
>
> I did a bit of experimenting, and as far as I can tell, everything works as
> expected. Reference::get is already registered as having
> vmIntrinsics::_Reference_get as it's intrinsic_id, and that intrinsic id is
> handled by the various code processors, such as being in the dispatch table
> that @vnkozlov refreenced.
>
> All of Reference::get, Reference::refersTo, and Reference::clear are subtly
> different in the details of how they are implemented and intrinsified, because
> there are differences among them.
>
> refersTo is final, but needs different implementations for Reference and
> PhantomReference. That distinction is handled via nonpublic virtual
> refersToImpl. And those are implemented in terms of separate intrinsified
> private native methods because we found that C2 handling of intrinsified
> virtual native methods had problems. If not for that issue, the refersToImpl
> methods would be intrinsified virtual native methods.
>
> Reference::clear also needs clearImpl, but for a different reason. There are
> places where we want to directly call the implementation of Reference::clear,
> ignoring any overrides by derived application classes. That is accomplished by
> calling the virtual clearImpl, which has implementations in Reference and
> PhantomReference. And the clearImpls are implemented in terms of separate
> intrinsified private native methods because of the above mentioned C2 issue.
>
> The native implementations for clear call the same JVM_ReferenceClear helper
> function, which uses unknown_referent_no_keepalive to handle the distinction
> between weak and phantom strengths. If we just had the native implementation
> we could drop the PhantomReference override. But this operation also has
> intrinsic support, and the intrinsic makes use of the known strength.
>
> Reference::get already has long-standing intrinsic support. The only thing
> being ch...
I think I understand the intrinsic matchers machinery better now: it indeed
binds intrinsics to concrete methods. So virtual overrides are supposed to have
no difference. Also, I tried a few examples where I thought it should fail, and
it does not. I added some diagnostic checks around interpreter, and they do not
fail:
diff --git a/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
b/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
index bdc2ca908bd..255fa6443af 100644
--- a/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
+++ b/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
@@ -652,6 +652,21 @@ address
TemplateInterpreterGenerator::generate_Reference_get_entry(void) {
// rdx: scratch
// rdi: scratch
+ if (UseNewCode) {
+ Label L_good;
+ __ push(rscratch1);
+ __ push(rscratch2);
+
+ __ load_klass(rscratch1, rax, rscratch2);
+ __ cmpb(Address(rscratch1,
in_bytes(InstanceKlass::reference_type_offset())), REF_PHANTOM);
+ __ jccb(Assembler::notEqual, L_good);
+ __ stop("PHANTOM REFERENCE CALLED WITH INTERPRETER REFERENCE.GET
INTRINSIC");
+ __ bind(L_good);
+
+ __ pop(rscratch2);
+ __ pop(rscratch1);
+ }
+
// Load the value of the referent field.
const Address field_address(rax, referent_offset);
__ load_heap_oop(rax, field_address, /*tmp1*/ rbx, ON_WEAK_OOP_REF);
I tried C2 IR test, and it works fine: we return constant `0` for
`PhantomReferences`, like we would expect. I am going to RFR that test
separately.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24315#discussion_r2039300718
