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

[Zoltán Majó]

Thank you, Vladimir, for the review!

Best regards,


Zoltan


On 07/28/2015 04:12 PM, Vladimir Kozlov wrote:
> 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/>
>>>>>
>>>>
>>