On Fri, 11 Apr 2025 10:52:03 GMT, Aleksey Shipilev <sh...@openjdk.org> wrote:
>> 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.
>>
> ...
>
> 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, [JDK-8354417](https://bugs.openjdk.org/browse/JDK-8354417).
>
> That said, I am still not 100% sure there is no bug, because I see intrinsics
> like `Object.hashCode` do seem to handle virtual cases.
Quoting from https://bugs.openjdk.org/browse/JDK-8271862
> However, Object.clone() and Object.hashCode() are also virtual, native and
> @IntrinsicCandidate, and these get special treatment by C2 to get the
> intrinsic generated more optimally. It's not clear to me if we should do
> something similar with refersTo0() or if the above workaround should be
> considered good enough.
For Reference.refersTo (and later, Reference.clear) we did the workaround,
avoiding methods that had all 3
properties. Similarly for Reference.get here. But it might be worthwhile
looking into this issue more carefully
and generally, both to make sure there aren't other affected intrinsics, and to
either solve that case better or
complain when it arises.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24315#discussion_r2094884157
