Re: [10] RFR 8186046 Minimal ConstantDynamic support

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR 8186046 Minimal ConstantDynamic support

Dmitry Samersoff-3
Paul,

templateTable_x86.cpp:

 564   const Register flags = rcx;
 565   const Register rarg = NOT_LP64(rcx) LP64_ONLY(c_rarg1);

Should we use another register for rarg under NOT_LP64 ?

-Dmitry


On 10/26/2017 08:03 PM, Paul Sandoz wrote:

> Hi,
>
> Please review the following patch for minimal dynamic constant support:
>
>   http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/>
>
>   https://bugs.openjdk.java.net/browse/JDK-8186046 <https://bugs.openjdk.java.net/browse/JDK-8186046>
>   https://bugs.openjdk.java.net/browse/JDK-8186209 <https://bugs.openjdk.java.net/browse/JDK-8186209>
>
> This patch is based on the JDK 10 unified HotSpot repository. Testing so far looks good.
>
> By minimal i mean just the support in the runtime for a dynamic constant pool entry to be referenced by a LDC instruction or a bootstrap method argument. Much of the work leverages the foundations built by invoke dynamic but is arguably simpler since resolution is less complex.
>
> A small set of bootstrap methods will be proposed as a follow on issue for 10 (these are currently being refined in the amber repository).
>
> Bootstrap method invocation has not changed (and the rules are the same for dynamic constants and indy). It is planned to enhance this in a further major release to support lazy resolution of bootstrap method arguments.
>
> The CSR for the VM specification is here:
>
>   https://bugs.openjdk.java.net/browse/JDK-8189199 <https://bugs.openjdk.java.net/browse/JDK-8189199>
>
> the j.l.invoke package documentation was also updated but please consider the VM specification as the definitive "source of truth" (we may clean up this area further later on so it becomes more informative, and that may also apply to duplicative text on MethodHandles/VarHandles).
>
> Any AoT-related work will be deferred to a future release.
>
> —
>
> This patch only supports x64 platforms. There is a small set of changes specific to x64 (specifically to support null and primitives constants, as prior to this patch null was used as a sentinel for resolution and certain primitives types would never have been encountered, such as say byte).
>
> We will need to follow up with the SPARC platform and it is hoped/anticipated that OpenJDK members responsible for other platforms (namely ARM and PPC) will separately provide patches.
>
> —
>
> Many of tests rely on an experimental byte code API that supports the generation of byte code with dynamic constants.
>
> One test uses class file bytes produced from a modified version of asmtools.  The modifications have now been pushed but a new version of asmtools need to be rolled into jtreg before the test can operate directly on asmtools information rather than embedding class file bytes directly in the test.
>
> —
>
> Paul.
>

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR 8186046 Minimal ConstantDynamic support

Dmitry Samersoff-3
Paul and Frederic,

Thank you.

One more question. Do we need to call verify_oop below?

509   { // Check for the null sentinel.
...
517      xorptr(result, result);  // NULL object reference
...

521   if (VerifyOops) {
522      verify_oop(result);
523   }

-Dmitry


On 31.10.2017 00:56, Frederic Parain wrote:

> I’m seeing no issue with rcx being aliased in this code.
>
> Fred
>
>> On Oct 30, 2017, at 15:44, Paul Sandoz <[hidden email]> wrote:
>>
>> Hi,
>>
>> Thanks for reviewing.
>>
>>> On 30 Oct 2017, at 11:05, Dmitry Samersoff <[hidden email]> wrote:
>>>
>>> Paul,
>>>
>>> templateTable_x86.cpp:
>>>
>>> 564   const Register flags = rcx;
>>> 565   const Register rarg = NOT_LP64(rcx) LP64_ONLY(c_rarg1);
>>>
>>> Should we use another register for rarg under NOT_LP64 ?
>>>
>>
>> I think it should be ok, it i ain’t an expert here on the interpreter and the calling conventions, so please correct me.
>>
>> Some more context:
>>
>> +  const Register flags = rcx;
>> +  const Register rarg = NOT_LP64(rcx) LP64_ONLY(c_rarg1);
>> +  __ movl(rarg, (int)bytecode());
>>
>> The current bytecode code is loaded into “rarg”
>>
>> +  call_VM(obj, CAST_FROM_FN_PTR(address, InterpreterRuntime::resolve_ldc), rarg);
>>
>> Then “rarg" is the argument to the call to InterpreterRuntime::resolve_ldc, after which it is no longer referred to.
>>
>> +#ifndef _LP64
>> +  // borrow rdi from locals
>> +  __ get_thread(rdi);
>> +  __ get_vm_result_2(flags, rdi);
>> +  __ restore_locals();
>> +#else
>> +  __ get_vm_result_2(flags, r15_thread);
>> +#endif
>>
>> The result from the call is then loaded into flags.
>>
>> So i don’t think it matters in this case if rcx is aliased.
>>
>> Paul.
>>
>>> -Dmitry
>>>
>>>
>>> On 10/26/2017 08:03 PM, Paul Sandoz wrote:
>>>> Hi,
>>>>
>>>> Please review the following patch for minimal dynamic constant support:
>>>>
>>>> http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/>
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8186046 <https://bugs.openjdk.java.net/browse/JDK-8186046>
>>>> https://bugs.openjdk.java.net/browse/JDK-8186209 <https://bugs.openjdk.java.net/browse/JDK-8186209>
>>>>
>>>> This patch is based on the JDK 10 unified HotSpot repository. Testing so far looks good.
>>>>
>>>> By minimal i mean just the support in the runtime for a dynamic constant pool entry to be referenced by a LDC instruction or a bootstrap method argument. Much of the work leverages the foundations built by invoke dynamic but is arguably simpler since resolution is less complex.
>>>>
>>>> A small set of bootstrap methods will be proposed as a follow on issue for 10 (these are currently being refined in the amber repository).
>>>>
>>>> Bootstrap method invocation has not changed (and the rules are the same for dynamic constants and indy). It is planned to enhance this in a further major release to support lazy resolution of bootstrap method arguments.
>>>>
>>>> The CSR for the VM specification is here:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8189199 <https://bugs.openjdk.java.net/browse/JDK-8189199>
>>>>
>>>> the j.l.invoke package documentation was also updated but please consider the VM specification as the definitive "source of truth" (we may clean up this area further later on so it becomes more informative, and that may also apply to duplicative text on MethodHandles/VarHandles).
>>>>
>>>> Any AoT-related work will be deferred to a future release.
>>>>
>>>> —
>>>>
>>>> This patch only supports x64 platforms. There is a small set of changes specific to x64 (specifically to support null and primitives constants, as prior to this patch null was used as a sentinel for resolution and certain primitives types would never have been encountered, such as say byte).
>>>>
>>>> We will need to follow up with the SPARC platform and it is hoped/anticipated that OpenJDK members responsible for other platforms (namely ARM and PPC) will separately provide patches.
>>>>
>>>> —
>>>>
>>>> Many of tests rely on an experimental byte code API that supports the generation of byte code with dynamic constants.
>>>>
>>>> One test uses class file bytes produced from a modified version of asmtools.  The modifications have now been pushed but a new version of asmtools need to be rolled into jtreg before the test can operate directly on asmtools information rather than embedding class file bytes directly in the test.
>>>>
>>>> —
>>>>
>>>> Paul.
>>>>
>>>
>>
>


Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR 8186046 Minimal ConstantDynamic support

Dmitry Samersoff-3
Paul,

Thank you!

-Dmitry


On 31.10.2017 20:32, Paul Sandoz wrote:

>
>> On 31 Oct 2017, at 05:58, Dmitry Samersoff <[hidden email]> wrote:
>>
>> Paul and Frederic,
>>
>> Thank you.
>>
>> One more question. Do we need to call verify_oop below?
>>
>> 509   { // Check for the null sentinel.
>> ...
>> 517      xorptr(result, result);  // NULL object reference
>> ...
>>
>> 521   if (VerifyOops) {
>> 522      verify_oop(result);
>> 523   }
>>
>
> I believe it’s harmless.
>
> When the flag is on it eventually results in a call to the stub generated by generate_verify_oop:
>
> http://hg.openjdk.java.net/jdk10/hs/file/tip/src/hotspot/cpu/x86/stubGenerator_x86_64.cpp#l1023
>
>     // make sure object is 'reasonable'
>     __ testptr(rax, rax);
>     __ jcc(Assembler::zero, exit); // if obj is NULL it is OK
>
> If the oop is null the verification will exit safely.
>
> Paul.
>
>> -Dmitry
>>
>>
>> On 31.10.2017 00:56, Frederic Parain wrote:
>>> I’m seeing no issue with rcx being aliased in this code.
>>>
>>> Fred
>>>
>>>> On Oct 30, 2017, at 15:44, Paul Sandoz <[hidden email]> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Thanks for reviewing.
>>>>
>>>>> On 30 Oct 2017, at 11:05, Dmitry Samersoff <[hidden email]> wrote:
>>>>>
>>>>> Paul,
>>>>>
>>>>> templateTable_x86.cpp:
>>>>>
>>>>> 564   const Register flags = rcx;
>>>>> 565   const Register rarg = NOT_LP64(rcx) LP64_ONLY(c_rarg1);
>>>>>
>>>>> Should we use another register for rarg under NOT_LP64 ?
>>>>>
>>>>
>>>> I think it should be ok, it i ain’t an expert here on the interpreter and the calling conventions, so please correct me.
>>>>
>>>> Some more context:
>>>>
>>>> +  const Register flags = rcx;
>>>> +  const Register rarg = NOT_LP64(rcx) LP64_ONLY(c_rarg1);
>>>> +  __ movl(rarg, (int)bytecode());
>>>>
>>>> The current bytecode code is loaded into “rarg”
>>>>
>>>> +  call_VM(obj, CAST_FROM_FN_PTR(address, InterpreterRuntime::resolve_ldc), rarg);
>>>>
>>>> Then “rarg" is the argument to the call to InterpreterRuntime::resolve_ldc, after which it is no longer referred to.
>>>>
>>>> +#ifndef _LP64
>>>> +  // borrow rdi from locals
>>>> +  __ get_thread(rdi);
>>>> +  __ get_vm_result_2(flags, rdi);
>>>> +  __ restore_locals();
>>>> +#else
>>>> +  __ get_vm_result_2(flags, r15_thread);
>>>> +#endif
>>>>
>>>> The result from the call is then loaded into flags.
>>>>
>>>> So i don’t think it matters in this case if rcx is aliased.
>>>>
>>>> Paul.
>>>>
>>>>> -Dmitry
>>>>>
>>>>>
>>>>> On 10/26/2017 08:03 PM, Paul Sandoz wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review the following patch for minimal dynamic constant support:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/>
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8186046 <https://bugs.openjdk.java.net/browse/JDK-8186046>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8186209 <https://bugs.openjdk.java.net/browse/JDK-8186209>
>>>>>>
>>>>>> This patch is based on the JDK 10 unified HotSpot repository. Testing so far looks good.
>>>>>>
>>>>>> By minimal i mean just the support in the runtime for a dynamic constant pool entry to be referenced by a LDC instruction or a bootstrap method argument. Much of the work leverages the foundations built by invoke dynamic but is arguably simpler since resolution is less complex.
>>>>>>
>>>>>> A small set of bootstrap methods will be proposed as a follow on issue for 10 (these are currently being refined in the amber repository).
>>>>>>
>>>>>> Bootstrap method invocation has not changed (and the rules are the same for dynamic constants and indy). It is planned to enhance this in a further major release to support lazy resolution of bootstrap method arguments.
>>>>>>
>>>>>> The CSR for the VM specification is here:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8189199 <https://bugs.openjdk.java.net/browse/JDK-8189199>
>>>>>>
>>>>>> the j.l.invoke package documentation was also updated but please consider the VM specification as the definitive "source of truth" (we may clean up this area further later on so it becomes more informative, and that may also apply to duplicative text on MethodHandles/VarHandles).
>>>>>>
>>>>>> Any AoT-related work will be deferred to a future release.
>>>>>>
>>>>>> —
>>>>>>
>>>>>> This patch only supports x64 platforms. There is a small set of changes specific to x64 (specifically to support null and primitives constants, as prior to this patch null was used as a sentinel for resolution and certain primitives types would never have been encountered, such as say byte).
>>>>>>
>>>>>> We will need to follow up with the SPARC platform and it is hoped/anticipated that OpenJDK members responsible for other platforms (namely ARM and PPC) will separately provide patches.
>>>>>>
>>>>>> —
>>>>>>
>>>>>> Many of tests rely on an experimental byte code API that supports the generation of byte code with dynamic constants.
>>>>>>
>>>>>> One test uses class file bytes produced from a modified version of asmtools.  The modifications have now been pushed but a new version of asmtools need to be rolled into jtreg before the test can operate directly on asmtools information rather than embedding class file bytes directly in the test.
>>>>>>
>>>>>> —
>>>>>>
>>>>>> Paul.
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>