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

[Vladimir Kozlov]

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.
>>>>>
>>>>>
>>>
>