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

[Vladimir Kozlov]

This looks good.

Thanks,
Vladimir

On 3/2/12 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/ <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
>