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

[Zoltán Majó]

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.

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

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

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.