[9] RFR(M): 8078554: Compiler: implement ranges (optionally constraints) for those flags that have them missing
Zoltán Majó
zoltan.majo at oracle.com
Mon Oct 12 08:06:00 UTC 2015
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.
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>> 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/%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
>>>>> 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/%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/%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.
>>
>
More information about the hotspot-compiler-dev
mailing list