RFR: 8164358: [JVMCI] expose Hotspot intrinsics and HotSpotIntrinsicCandidate info to JVMCI compilers

[Doug Simon]

> On 24 Aug 2016, at 00:44, Christian Thalinger <cthalinger at twitter.com> wrote:
> 
> 
>> On Aug 23, 2016, at 11:41 AM, Doug Simon <doug.simon at oracle.com> wrote:
>> 
>> While trying to push this, we hit a C compiler crash in read_configuration(). In the hope that it’s related to the expansion of the VM_INTRINSICS_DO macro, I’ve refactored this code into a separate method:
>> 
>> http://cr.openjdk.java.net/~dnsimon/8164358v3/
> 
> +    VMIntrinsicMethod::set_id(vmIntrinsicMethod, vmIntrinsics::id);       \
> +      vmIntrinsics->obj_at_put(index++, vmIntrinsicMethod());             \
> +  }
> I was talking about the spaces at the beginning of the line.  Should’ve asked for a new webrev ;-)

Damn, sorry about that! Will clean it up in a subsequent bug.

-Doug

>>> On 22 Aug 2016, at 22:31, Doug Simon <doug.simon at oracle.com> wrote:
>>> 
>>> 
>>>> On 22 Aug 2016, at 18:58, Christian Thalinger <cthalinger at twitter.com> wrote:
>>>> 
>>>>> 
>>>>> On Aug 19, 2016, at 11:39 AM, Doug Simon <doug.simon at oracle.com> wrote:
>>>>> 
>>>>>> 
>>>>>> On 19 Aug 2016, at 23:18, Christian Thalinger <cthalinger at twitter.com> wrote:
>>>>>> 
>>>>>> 
>>>>>>> On Aug 19, 2016, at 5:01 AM, Doug Simon <doug.simon at oracle.com> wrote:
>>>>>>> 
>>>>>>> I changed it to only try reuse the declaringClass string (which has the most overlap):
>>>>>>> 
>>>>>>> http://cr.openjdk.java.net/~dnsimon/8164358v2/
>>>>>> 
>>>>>> Looks good.  The only comment I have is about:
>>>>>> VMField::klass()->initialize(thread);
>>>>>> VMFlag::klass()->initialize(thread);
>>>>>> 
>>>>>> +  VMIntrinsicMethod::klass()->initialize(thread);
>>>>>> We should use CHECK_NULL here too.  Sure, it's very unlikely these fail but better safe than sorry.
>>>>> 
>>>>> Thanks for catching this! I don’t think I’ll ever develop the eye you have for consistent exception propagation in VM code. I’ve spent too much time spent in a language where this is automatic ;-)
>>>> 
>>>> I hope I’ll forget soon… :-)
>>>> 
>>>>> 
>>>>> I’ve made the change in the v2 web rev.
>>>> 
>>>> Yes, this looks good.  One thing I noticed is somehow indent is off here:
>>>> +    VMIntrinsicMethod::set_descriptor(vmIntrinsicMethod, sig_str());      \
>>>> +    VMIntrinsicMethod::set_id(vmIntrinsicMethod, vmIntrinsics::id);       \
>>>> +      vmIntrinsics->obj_at_put(index++, vmIntrinsicMethod());               \
>>>> +  }
>>>> No need for a new webrev.
>>> 
>>> Ok - thanks for another detailed review!
>>> 
>>> -Doug
>>> 
>>>>>>>> On 18 Aug 2016, at 23:59, Doug Simon <doug.simon at oracle.com> wrote:
>>>>>>>> 
>>>>>>>> 
>>>>>>>>> On 18 Aug 2016, at 20:01, Christian Thalinger <cthalinger at twitter.com> wrote:
>>>>>>>>> 
>>>>>>>>> src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/CompilerToVM.java
>>>>>>>>> 
>>>>>>>>>  *         [String name, Long value, ...] vmAddresses,
>>>>>>>>> 
>>>>>>>>> +     *         [String name, Long value, ...] vmAddresses,
>>>>>>>>> 
>>>>>>>>>  *         VMFlag[] vmFlags
>>>>>>>>> 
>>>>>>>>> +     *         VMIntrinsicMethod[] vmIntrinsics
>>>>>>>>> Looks like a copy-paste problem.
>>>>>>>> 
>>>>>>>> Oops! Will fix.
>>>>>>>> 
>>>>>>>>> src/share/vm/jvmci/jvmciCompilerToVM.cpp
>>>>>>>>> 
>>>>>>>>> Why do we have to do the prev_##var##_sid == sid dance?  I guess it’s some optimization but does it really matter?
>>>>>>>> 
>>>>>>>> It cuts down the number Strings allocated by about 35% (900 vs 582). With full string interning, the savings is around 55% (415) but the overhead of using a HashSet from VM code is prohibitive. I won’t argue strongly that it makes a big difference but since the one-element cache (per VMIntrinsicMethod string field) is fast and cheap, I don’t see why not to use it. The alternative code would look something like this:
>>>>>>>> 
>>>>>>>> objArrayHandle vmIntrinsics = oopFactory::new_objArray(VMIntrinsicMethod::klass(), (vmIntrinsics::ID_LIMIT - 1), CHECK_NULL);
>>>>>>>> int index = 0;
>>>>>>>> #define VM_SYMBOL_TO_STRING(s) java_lang_String::create_from_symbol(vmSymbols::symbol_at(vmSymbols::VM_SYMBOL_ENUM_NAME(s)), THREAD)
>>>>>>>> #define VM_INTRINSIC_INFO(id, kls, name, sig, ignore_fcode) {                  \
>>>>>>>> instanceHandle vmIntrinsicMethod = InstanceKlass::cast(VMIntrinsicMethod::klass())->allocate_instance_handle(CHECK_NULL); \
>>>>>>>> Handle kls_string = VM_SYMBOL_TO_STRING(kls);                              \
>>>>>>>> Handle name_string = VM_SYMBOL_TO_STRING(name);                            \
>>>>>>>> Handle sig_string = VM_SYMBOL_TO_STRING(sig);                              \
>>>>>>>> VMIntrinsicMethod::set_declaringClass(vmIntrinsicMethod, kls_string());    \
>>>>>>>> VMIntrinsicMethod::set_name(vmIntrinsicMethod, name_string());             \
>>>>>>>> VMIntrinsicMethod::set_descriptor(vmIntrinsicMethod, sig_string());        \
>>>>>>>> VMIntrinsicMethod::set_id(vmIntrinsicMethod, vmIntrinsics::id);            \
>>>>>>>> vmIntrinsics->obj_at_put(index++, vmIntrinsicMethod());                    \
>>>>>>>> }
>>>>>>>> 
>>>>>>>> VM_INTRINSICS_DO(VM_INTRINSIC_INFO, VM_SYMBOL_IGNORE, VM_SYMBOL_IGNORE, VM_SYMBOL_IGNORE, VM_ALIAS_IGNORE)
>>>>>>>> #undef VM_SYMBOL_TO_STRING
>>>>>>>> #undef VM_INTRINSIC_INFO
>>>>>>>> 
>>>>>>>> Better?
>>>>>>>> 
>>>>>>>> -Doug
>>>>>>>> 
>>>>>>>>> If we don’t have to do that I would prefer is that macro would be a method (or two).
>>>>>>>>> 
>>>>>>>>>> On Aug 18, 2016, at 6:50 AM, Doug Simon <doug.simon at oracle.com> wrote:
>>>>>>>>>> 
>>>>>>>>>> Please review this change which allows JVMCI compilers to see what intrinsics are defined by the VM and which methods have been annotated by HotSpotIntrisicCandidate.
>>>>>>>>>> 
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8164358
>>>>>>>>>> http://cr.openjdk.java.net/~dnsimon/8164358/
>>>>>>>>>> 
>>>>>>>>>> Thanks,
>>>>>>>>>> 
>>>>>>>>>> -Doug
>>> 
>> 
>