[9] RFR(M): 8130832: Extend the WhiteBox API to provide information about the availability of compiler intrinsics

[Vladimir Kozlov]

This looks good.

Thanks,
Vladimir

On 7/28/15 3:17 AM, Zoltán Majó wrote:
> Hi Vladimir,
>
>
> thank you for the feedback!
>
> On 07/27/2015 06:38 PM, Vladimir Kozlov wrote:
>> WB method isIntrinsicAvailableForMethod0 has parameter
>> compilationContext. So I don't think you need
>> is_intrinsic_available(methodHandle method) variant - it is not used
>> from WB.
>
> You're right, I removed that variant.
>
>> You left is_intrinsic_available*_for* in comments in abstractCompiler.hpp
>
> Updated. I also updated the text in the comments at other places so that
> they are more accurate than before.
>
>>
>> Following the same naming logic I would suggest to remove "ForMethod"
>> in WB new methods names.
>
> Updated the method names as well.
>
>>
>> Missing 'virtual' in c2compiler.hpp
>
> Updated as well.
>
>>
>> Leftover line in c2compiler.cpp:
>>
>> +   //return Compile::is_intrinsic_available(method,
>> compilation_context, false);
>
> Thank you for spotting that, removed it.
>
>>
>> Indention is off - keep following lines at the same level as
>> !compilation_context (one space after "("):
>>
>> +       (!compilation_context.is_null() &&
>> +           CompilerOracle::has_option_value(compilation_context,
>> "DisableIntrinsic", disable_intr) &&
>> +           strstr(disable_intr, vmIntrinsics::name_at(id)) != NULL)
>
> Updated the indentation.
>
>>
>> Remove 'return false' because following InlineUnsafeOps check disable
>> some intrinsics:
>>
>> +     case vmIntrinsics::_Reference_get:
>> +       return false;
>> +       break;
>
> Oh, I missed that! Updated.
>
>>
>> Also I would prefer vmIntrinsics::is_disabled_by_flags() was called
>> from compiler's is_intrinsic_disabled_by_flag() flag.
>
> I moved the call to the compiler-specific is_intrinsic_disabled_by_flag
> methods.
>
>>
>> Additionally I think we should not have difference in behavior of
>> is_intrinsic_disabled_by_flag() in C1 and C2. Both compilers should
>> follow the same rules. If we disable intrinsic on command line - C1
>> should not intrinsify it. The only difference should be in supported
>> intrinsics. If you think it is big change - file an other bug (it is
>> bug, not rfe) to fix it separately.
>
> Yes, I also think it makes sense that flags behave the same way for all
> compilers. But I would like to keep that as a separate issue, partly
> because I haven't figured out all details yet for implementing a fix and
> partly because the current issue is related to some "critical" nightly
> failures we have.
>
> So I filed JDK-8132457: "Unify command-line flags controlling the usage
> of compiler intrinsics" for addressing inconsistencies in processing
> intrinsic-related command-line flags. I would like to push the newest
> webrev (if you are fine with it) and then continue with JDK-8132457. I
> hope that is OK.
>
> Here is the newest webrev:
> - top: http://cr.openjdk.java.net/~zmajo/8130832/top/webrev.04/
> - hotspot: http://cr.openjdk.java.net/~zmajo/8130832/hotspot/webrev.04/
>
> Testing:
> - JPRT (testset "hotspot" + tests in compiler/intrinsics/mathexact); all
> tests pass.
>
> Thank you and best regards,
>
>
> Zoltan
>
>
>>
>> Thanks,
>> Vladimir
>>
>> On 7/22/15 12:59 AM, Zoltán Majó wrote:
>>> Hi John,
>>>
>>>
>>> On 07/21/2015 09:02 PM, John Rose wrote:
>>>> Yes, that will work, and I think it is cleaner than what we had
>>>> before, as well as providing the new required functionality.
>>>>
>>>> Reviewed; please get a second reviewer.
>>>
>>> thank you for the review! I'll ask Vladimir K., maybe he has time to
>>> look at the newest webrev.
>>>
>>>>
>>>> — John
>>>>
>>>> P.S. If the unit tests want to test (via the whitebox API) whether an
>>>> intrinsic was compiled successfully, we might want to expose
>>>> Compile::gather_intrinsic_statistics, etc.  But not in this change set.
>>>
>>> That is an interesting idea. We'd also have to see if current tests
>>> require such functionality or if SQE plans to add tests requiring that
>>> functionality.
>>>
>>>>
>>>> P.P.S. As I think I said before, I wish we had a way to consolidate
>>>> the switch statements further (into vmSymbols.hpp).  But I don't see a
>>>> clean way to do it.
>>>
>>> Yes, that would be nice. I've not seen a good way to do that, partly
>>> because inconsistencies between the way C1 and C2 depends on the value
>>> of command-line flags.
>>>
>>> Thank you!
>>>
>>> Best regards,
>>>
>>>
>>> Zoltan
>>>
>>>>
>>>> On Jul 21, 2015, at 8:19 AM, Zoltán Majó <zoltan.majo at oracle.com
>>>> <mailto:zoltan.majo at oracle.com>> wrote:
>>>>>
>>>>> Here is the newest webrev:
>>>>> - top:http://cr.openjdk.java.net/~zmajo/8130832/top/
>>>>> <http://cr.openjdk.java.net/%7Ezmajo/8130832/top/>
>>>>> -
>>>>> hotspot:http://cr.openjdk.java.net/~zmajo/8130832/hotspot/webrev.03/
>>>>> <http://cr.openjdk.java.net/%7Ezmajo/8130832/hotspot/webrev.03/>
>>>>
>>>
>