Request for reviews (S): 6658428: C2 doesn't inline java method if corresponding intrinsic failed to inline

[Nils Eliasson]

sorry, missed that.

Here is the a new one, with Johns suggestion too.

http://cr.openjdk.java.net/~neliasso/6658428.01/webrev.04/ 
<http://cr.openjdk.java.net/%7Eneliasso/6658428.01/webrev.04/>

//N

Vladimir Kozlov skrev 2012-02-24 18:23:
> Looks good.
>
> I asked before to move msg definition under the following check where 
> it is used:
>
> +  if (PrintIntrinsics || PrintInlining NOT_PRODUCT( || 
> PrintOptoInlining) ) {
>      if (jvms->has_method()) {
> +      const char* msg = is_virtual() ? "failed to inline (intrinsic, 
> virtual)" : "failed to inline (intrinsic)";
>
> Thanks,
> Vladimir
>
> On 2/24/12 1:48 AM, Nils Eliasson wrote:
>> +1 for restructuring. A common table with intrinsic_id, matcher and 
>> expander. Perhaps even c1/c2 agnostic.
>>
>> Also noticed that there are no popcount intrinsic for c1. Adding that 
>> to my learning c1 task list.
>>
>> New webrev here: 
>> http://cr.openjdk.java.net/~neliasso/6658428.01/webrev.03/
>> <http://cr.openjdk.java.net/%7Eneliasso/6658428.01/webrev.03/>
>>
>> Thanks!
>>
>> //N
>>
>> Tom Rodriguez skrev 2012-02-22 19:36:
>>> On Feb 22, 2012, at 5:27 AM, Nils Eliasson wrote:
>>>
>>>> Thanks Vladimir and Tom for the feedback,
>>>>
>>>> I agree with removing the out-of-line part.
>>>>
>>>> Removing the UsePopCountInstruction-guard in library_call also 
>>>> makes it nicer and cleaner, but noting that we in a few rare cases 
>>>> create call generators for intrinsics that always return false.
>>> Actually what I meant was to propagate the appropriate 
>>> has_match_rule test up into make_vm_intrinsic. So something more like:
>>>
>>>    case vmIntrinsics::_bitCount_i:
>>>      if (!Matcher::has_match_rule(Op_PopCountI)) return NULL;
>>>      break;
>>>    case vmIntrinsics::_bitCount_l:
>>>      if (!Matcher::has_match_rule(Op_PopCountL)) return NULL;
>>>      break;
>>>    case vmIntrinsics::_numberOfLeadingZeros_i:
>>>      if (!Matcher::match_rule_supported(Op_CountLeadingZerosI))) 
>>> return NULL;
>>>      break;
>>>    …
>>>
>>> I think that will take into account the various flags properly 
>>> without having extra ifdefs.
>>>
>>> There's a more major restructuring of the intrinsics lurking in all 
>>> this I think, which I'm not advocating you do as part of this 
>>> change.  It's always seemed confusing to me that make_vm_intrinsics 
>>> doesn't include all the intrinsics, and the duplication of the 
>>> static guards like has_match_rule and other globals flags in 
>>> make_vm_intrinsics and the actual inliner as well seems wasteful.
>>>
>>> The other changes look fine.
>>>
>>> tom
>>>
>>>> http://cr.openjdk.java.net/~neliasso/6658428.01/webrev.01/
>>>>
>>>> Skiping reviewboard for now, doesn't seem to work well with mq and 
>>>> cygwin.
>>>>
>>>> //N
>>>>
>>>> Tom Rodriguez skrev 2012-02-21 20:05:
>>>>> On Feb 21, 2012, at 2:26 AM, Nils Eliasson wrote:
>>>>>
>>>>>
>>>>>> http://cr.openjdk.java.net/~neliasso/6658428.01/webrev.00/
>>>>>>
>>>>>>
>>>>>> 6658428: C2 doesn't inline java method if corresponding intrinsic 
>>>>>> failed to inline.
>>>>>>
>>>>>> Allow intrinsic inline to fallback to plain java inline case. 
>>>>>> Changing some intrinsics to not be created when they cant be used 
>>>>>> and adding some print inline changes.
>>>>>>
>>>>> I don't think you need to try inline and out of line separately.  
>>>>> I think the old code forced out of line because that was the only 
>>>>> way to suppress the use of the intrinsic.  So remove the 
>>>>> try_inline = false line and just add the extra false argument to 
>>>>> the existing call.  It should use inline if it's appropriate or 
>>>>> out of line if not.
>>>>>
>>>>> In library_call.cpp I think it would be better to stop using 
>>>>> !UsePopCountInstruction to guard those intrinsics and use the 
>>>>> appropriate Matcher::has_match_rule for each one.  That should 
>>>>> take into account the platform dependent meaning of 
>>>>> UsePopCountIntstruction.
>>>>>
>>>>> tom
>>>>>
>>>>>
>>>>>> And I will need someone to submit it.
>>>>>>
>>>>>> Thanks,
>>>>>> Nils E.
>>>>>>
>>>>>>
>>>>
>>


-- 
Oracle <http://www.oracle.com>
Nils Eliasson | Senior Member of Technical Staff
Oracle Java Platform Group, JVM Engineering
ORACLE Sweden

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20120302/adc0a527/attachment.html 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: oracle_sig_logo.gif
Type: image/gif
Size: 658 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20120302/adc0a527/oracle_sig_logo.gif