[9] RFR(M): 8078554: Compiler: implement ranges (optionally constraints) for those flags that have them missing
Christian Thalinger
christian.thalinger at oracle.com
Fri Oct 9 21:33:55 UTC 2015
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?
> On Oct 8, 2015, at 5:52 AM, Zoltán Majó <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/
>>>
>>> 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
>>>> 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/
>>>>>> 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/
>>>>>
>>>>> 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/20151009/65b21044/attachment-0001.html>
More information about the hotspot-compiler-dev
mailing list