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

[Zoltán Majó]

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.