JDK-8217998: Remove method_type field associated with the appendix field of an indy or method handle call

[Lois Foltan]

On 2/6/2019 3:34 PM, Lois Foltan wrote:
>
> On 2/5/2019 6:01 PM, coleen.phillimore at oracle.com wrote:
>>
>> Hi Lois,  I like that there are no longer two resolved references for 
>> indy instructions, especially if it wasn't used.
>
> Thanks Coleen for the review!  More below.
>
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8217998.2/webrev/src/hotspot/share/interpreter/linkResolver.cpp.udiff.html 
>>
>>
>> I can't find a call where set_handle() passes false for the 
>> _has_local_sig field.
>
> There is no call to set_handle() that passes false for the 
> _has_local_sig field.  It is set to false initially in 
> CallInfo::set_common.  I considered not passing the "has_local_sig" 
> parameter to set_handle and just always set CallInfo::_has_local_sig 
> to true within that method but I am assuming John wanted to allow for 
> the possibility that it could be set to false in the future.
>
>>
>> Can you rename CallInfo::_has_local_signature, since it's not that 
>> much longer and reads better?
>>
>> + const bool has_local_sig = call_info.has_local_signature();
>> + assert(has_local_sig, "MHs and indy are always sig-poly");
>
> I'm confused.  The field currently within CallInfo is named 
> CallInfo::_has_local_sig.  Are you referring to the method 
> CallInfo::has_local_signature()?  Do you want that shortened?
>
>>
>> So why do you need this field if it's always true?  Was hoping for a 
>> cpCache bit back :)

Hi Coleen,

I looked over your review comments again and you are correct concerning 
the need for CallInfo::_has_local_sig.  It is always set to true for MHs 
and indy and thus really is not necessary for setting the local variable 
"has_local_sig" within the method 
ConstantPoolCacheEntry::set_method_handle_common().  I have removed 
CallInfo::_has_local_sig. 
ConstantPoolCacheEntry::has_local_signature_shift is still necessary, 
however.

Updated webrev at: 
http://cr.openjdk.java.net/~lfoltan/bug_jdk8217998.3/webrev/

Thanks for your review and help with this!
Lois

>
> It's not always true.
>
>>
>> Perhaps there's a future use?
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8217998.2/webrev/src/hotspot/share/oops/cpCache.hpp.udiff.html 
>>
>>
>> + has_local_signature_shift = 25, // (M) does the call site have a 
>> per-site signature (sig-poly methods)?
>>
>>
>> Can you make this an "S" vs. "M" and change the coment in the top of 
>> cpCache.hpp:
>>
>> // _flags     [tos|0|F=0|M|A|I|f|0|vf|indy_rf|000|00000|psize] (for 
>> method entries)
>> // bit length [ 4 |1| 1 |1|1|1|1|1|1 |-4--|--8--|--8--]
>
> Done, changed the "M" to "S" in both places.  The comment in the top 
> of cpCache.hpp does not refer to the method_type in any form, so 
> unclear as to how you want me to change it?  I am assuming you want me 
> to remove the "method_type info (t_section) from this comment?
>
> // _flags   = method type info (t section),
> //            virtual final bit (vfinal),
>
>
>>
>> And a line under this:
>>
>> // A      = call site has an appendix argument (loaded from resolved 
>> references)
>
> Not sure what you mean by this?
>
> Thanks,
> Lois
>
>>
>> Although I don't like this comment because it gets out of date.
>>
>> Thanks,
>> Coleen
>>
>> On 2/4/19 7:32 AM, Lois Foltan wrote:
>>> Please review this change that removes the unused method_type field 
>>> associated with the appendix field of an indy or method handle 
>>> call.  This is the first of a series of changes working towards 
>>> JDK-8210685: Bootstrap method consolidation.
>>>
>>> open webrev at: 
>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8217998.1/webrev/
>>> bug link: https://bugs.openjdk.java.net/browse/JDK-8217998
>>> co-contributors: Lois Foltan & John Rose
>>>
>>> Testing: hs-tier1-4 & jdk-tier1-3 (all platforms), hs-tier5-8 (linux 
>>> only), JCK vm & lang (linux only)
>>>
>>> Thanks,
>>> Lois
>>
>