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

[Zoltán Majó]

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