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

[Tom Rodriguez]

On Mar 2, 2012, at 1:54 AM, Nils Eliasson wrote:

> sorry, missed that. 
> 
> Here is the a new one, with Johns suggestion too.
> 
> http://cr.openjdk.java.net/~neliasso/6658428.01/webrev.04/

Looks good.  

tom

> 
> //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_sig_logo.gif>
> Nils Eliasson | Senior Member of Technical Staff
> Oracle Java Platform Group, JVM Engineering
> ORACLE Sweden 
>