[9] RFR(M): 8078554: Compiler: implement ranges (optionally constraints) for those flags that have them missing

Christian Thalinger christian.thalinger at oracle.com
Mon Oct 12 17:58:22 UTC 2015


> On Oct 11, 2015, at 10:06 PM, Zoltán Majó <zoltan.majo at oracle.com> wrote:
> 
> Hi,
> 
> 
> On 10/09/2015 11:33 PM, Christian Thalinger wrote:
>> After JEP 243 was integrated we fail one of the new tests with:
>> 
>> intx TypeProfileWidth=8 is outside the allowed range [ 0 ... 4 ]
>> 
>> The reason is that JVMCI can support more than 4 type profiles.  Currently the default is 8:
>> 
>> if (UseJVMCICompiler) {
>> if (FLAG_IS_DEFAULT(TypeProfileWidth)) {
>>        TypeProfileWidth = 8;
>>      }
>>    }
>> 
>> We should increase the range.  Not sure what a good number would be, though. Maybe 100 just to be safe?
> 
> 16 is already too large -- it triggers an assert on a platform we support. I'll look into extending the range to 8.

I didn’t try :-)  Thanks.

> 
> Best regards,
> 
> 
> Zoltan
> 
>> 
>>> On Oct 8, 2015, at 5:52 AM, Zoltán Majó <zoltan.majo at oracle.com <mailto:zoltan.majo at oracle.com><mailto:zoltan.majo at oracle.com <mailto:zoltan.majo at oracle.com>>> wrote:
>>> 
>>> Thank you, Tobias, for the review!
>>> 
>>> Best regards,
>>> 
>>> 
>>> Zoltán
>>> 
>>> On 10/08/2015 04:10 PM, Tobias Hartmann wrote:
>>>> Hi Zoltán,
>>>> 
>>>> On 08.10.2015 14:07, Zoltán Majó wrote:
>>>>> Hi Tobias,
>>>>> 
>>>>> 
>>>>> thank you for the feedback!
>>>>> 
>>>>> On 10/07/2015 03:38 PM, Tobias Hartmann wrote:
>>>>>> Hi Zoltan,
>>>>>> 
>>>>>> I had a look at your changes and just spotted some minor things:
>>>>>> 
>>>>>> globals_sparc.hpp:
>>>>>> - I think there is a '\' missing in line 119
>>>>> thank you for spotting that!
>>>>> 
>>>>>> globals_x86.hpp:
>>>>>> - Isn't this also a compiler flag we should add range checks for?
>>>>>>  136   product(uintx, RTMRetryCount, 5,
>>>>> JEP 245 considers it as a runtime flag and JDK-8078556 "Runtime: implement ranges..." [1] will take care of it. But you are right, that flag could be also considered a compiler flag.
>>>> Okay, thanks for pointing that out.
>>>> 
>>>>>> commandLineFlagConstraintsCompiler.cpp:
>>>>>> - I think there is a "rule" that the include statements should be in alphabetical order
>>>>> Yes, I think there is such a rule (or convention). I diverged from the rule because the include of code/relocInfo.hpp depends on 'os', 'vm_page_size', and 'Metadata'. Therefore, "oops/metadata.hpp" and "runtime/os.hpp" must be included before relocInfo.hpp (otherwise the Solaris compiler complains). The remaining includes are ordered alphabetically.
>>>> Okay, makes sense.
>>>> 
>>>>>> - the indentation is wrong here:
>>>>>>  179           return Flag::VIOLATES_CONSTRAINT;
>>>>> I updated the indentation.
>>>>> 
>>>>> Here is the updated webrev:
>>>>> http://cr.openjdk.java.net/~zmajo/8078554/webrev.02/ <http://cr.openjdk.java.net/~zmajo/8078554/webrev.02/><http://cr.openjdk.java.net/%7Ezmajo/8078554/webrev.02/ <http://cr.openjdk.java.net/%7Ezmajo/8078554/webrev.02/>>
>>>>> 
>>>>> I re-tested the updated webrev with JPRT (testset hotspot), all tests pass.
>>>> Looks good to me (not a Reviewer).
>>>> 
>>>> Best,
>>>> Tobias
>>>> 
>>>> 
>>>>> Thank you and best regards,
>>>>> 
>>>>> 
>>>>> Zoltan
>>>>> 
>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8078556 <https://bugs.openjdk.java.net/browse/JDK-8078556>
>>>>>> Best,
>>>>>> Tobias
>>>>> 
>>>>>> On 06.10.2015 13:45, Zoltán Majó wrote:
>>>>>>> Hi Roland,
>>>>>>> 
>>>>>>> 
>>>>>>> thank you for the feedback!
>>>>>>> 
>>>>>>> On 10/02/2015 03:55 PM, Roland Westrelin wrote:
>>>>>>>> Hi Zoltan,
>>>>>>>> 
>>>>>>>>> Webrev:http://cr.openjdk.java.net/~zmajo/8078554/ <http://cr.openjdk.java.net/~zmajo/8078554/><http://cr.openjdk.java.net/%7Ezmajo/8078554/ <http://cr.openjdk.java.net/%7Ezmajo/8078554/>>
>>>>>>>> c2_globals.hpp
>>>>>>>> 
>>>>>>>> That one is not correct:
>>>>>>>>   461   product(intx, MaxNodeLimit, 80000,                               \
>>>>>>>>   462           "Maximum number of nodes")                               \
>>>>>>>>   463           range(1000, 80000)                                       \
>>>>>>>> 
>>>>>>>> I think the upper bound should be max_juint
>>>>>>> You are right that the limit of 80'000 is too conservative. But max_j*u*int as an upper bound would cause an overflow when parsing the flag's value, because on 32-bit machines intx is a 32-bit signed integer.
>>>>>>> 
>>>>>>> Using max_jint instead of max_j*u*int as an upper bound would not cause an overflow at parse time. However, in Parse::do_call() the maximum node limit is increased by 3 times for jsr292 users
>>>>>>> 
>>>>>>> C->set_max_node_limit(3*MaxNodeLimit);
>>>>>>> 
>>>>>>> If MaxNodeLimit == max_jint, this expression will overflow, I think.
>>>>>>> 
>>>>>>> So I set the limit to (max_jint / 3) in the updated webrev.
>>>>>>> 
>>>>>>> If we would set MaxNodeLimit to max_j*u*int / 3 (instead of max_jint / 3), the expression 3 * MaxNodeLimit would overflow as well. Changing the type of the flag from intx to uintx could let use use max_j*u*int / 3 as an upper bound, but that is most likely not worth the effort.
>>>>>>> 
>>>>>>>>   699   product(intx, LiveNodeCountInliningCutoff, 40000,                \
>>>>>>>>   700           "max number of live nodes in a method")                  \
>>>>>>>>   701           range(0, max_juint / 8)                                  \
>>>>>>>> 
>>>>>>>> Out of curiosity why max_juint / 8 (not that it makes much of a difference)?
>>>>>>> In Compile::inline_incrementally, the 80% of LiveNodeCountInliningCutoff is computed the following way:
>>>>>>> 
>>>>>>> if (low_live_nodes < (uint)LiveNodeCountInliningCutoff * 8 / 10) {
>>>>>>> 
>>>>>>> If LiveNodeCountInliningCutoff == max_juint, we'd have an overflow because of the multiplication by 8.
>>>>>>> 
>>>>>>>> arguments.cpp
>>>>>>>> 
>>>>>>>> 1099       Tier3InvokeNotifyFreqLog = 0;
>>>>>>>> 1100       Tier4InvocationThreshold = 0;
>>>>>>>> 
>>>>>>>> Why that change?
>>>>>>> I proposed that change because I misread the code. I reverted that change and also changed the range of all Tier*FreqLog flags from range(1, 30) to range(0, 30).
>>>>>>> 
>>>>>>>> globals.hp
>>>>>>>> 
>>>>>>>> 2870   product_pd(uintx, TypeProfileLevel,                               \
>>>>>>>> 2871           "=XYZ, with Z: Type profiling of arguments at call; "     \
>>>>>>>> 2872                      "Y: Type profiling of return value at call; " \
>>>>>>>> 2873                      "X: Type profiling of parameters to methods; " \
>>>>>>>> 2874           "X, Y and Z in 0=off ; 1=jsr292 only; 2=all methods")     \
>>>>>>>> 2875           range(0, 222)
>>>>>>>> 
>>>>>>>> Legal values are 0, 1, 2, 10, 11, 12, 100, 101, 102, 110, 111, 112 etc.
>>>>>>>> 
>>>>>>>> 70 is not for instance. So range(0, 222) is incorrect.
>>>>>>> I agree. I removed the range check and implemented a constraint function instead (TypeProfileLevelConstraintFunc).
>>>>>>> 
>>>>>>>> 2877   product(intx, TypeProfileArgsLimit,     2,                        \
>>>>>>>> 2878           "max number of call arguments to consider for type profiling")    \
>>>>>>>> 2879           range(0, 16)                                              \
>>>>>>>> 
>>>>>>>> 2880                                                                     \
>>>>>>>> 2881   product(intx, TypeProfileParmsLimit,    2,                        \
>>>>>>>> 2882           "max number of incoming parameters to consider for type profiling"\
>>>>>>>> 2883           ", -1 for all")                                           \
>>>>>>>> 2884           range(-1, 64)
>>>>>>>> 
>>>>>>>> Why 16 and 64?
>>>>>>> These are the largest values that work on all platforms we support.
>>>>>>> 
>>>>>>> Here is the updated webrev: http://cr.openjdk.java.net/~zmajo/8078554/webrev.01/ <http://cr.openjdk.java.net/~zmajo/8078554/webrev.01/><http://cr.openjdk.java.net/%7Ezmajo/8078554/webrev.01/ <http://cr.openjdk.java.net/%7Ezmajo/8078554/webrev.01/>>
>>>>>>> 
>>>>>>> I repeated the testing with JPRT. I also executed the currently disabled TestOptionsWithRanges.java test on all platforms we support. All tests pass.
>>>>>>> 
>>>>>>> Thank you and best regards,
>>>>>>> 
>>>>>>> 
>>>>>>> Zoltan
>>>>>>> 
>>>>>>>> Roland.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20151012/ebe48f4b/attachment-0001.html>


More information about the hotspot-compiler-dev mailing list