RFR(M): 8007288: Additional WB API for compiler's testing

Vladimir Kozlov vladimir.kozlov at oracle.com
Thu Apr 4 18:03:57 PDT 2013 

Don't change ciReplay.cpp now. File a new bug on hotspot/compiler 
because compilation level should be recorded by 
ciEnv::dump_replay_data() and used during replay.

It does not affect WB testing because it is only used during replay 
compilation.

 > The only place where used inaccurate 'comp_level' is
 > 'NonTieredCompPolicy::initial_compile_level', and I think it would be
 > better if it returns correct level.

So the problem is next #if which is always true for Server VM now:

#if defined(TIERED)
CompLevel_initial_compile   = CompLevel_full_profile

but NonTieredCompPolicy is selected (CompilationPolicyChoice==0) based 
on TieredCompilation flag.

Okay, you fix for NonTieredCompPolicy::initial_compile_level is correct.

Thanks,
Vladimir

On 4/4/13 10:13 AM, Igor Ignatyev wrote:
> I also found another place with inaccurate comp_level in
> 'ciReplay.cpp::process_compile()', there always uses
> 'CompLevel_full_optimization'.
>
> webrev: http://cr.openjdk.java.net/~iignatyev/8007288/webrev.03/
>
> Best regards,
> Igor Ignatyev
>
> On 04/04/2013 10:40 AM, Igor Ignatyev wrote:
>>>> Now, why you need new changes in compilationPolicy.hpp? It was not in
>>>> previous webrev. Yes, it may be correct but why?
>>
>> After I had look-see 'globalDefinitions' I mistakenly decided that
>> 'CompLevel_initial_compile' always is 'CompLevel_full_optimization' for
>> C2. So I said that 'comp_level' always equals 'CompLevel_highest_tier'
>> in 'CompileBroker::compile_method' for non-tiered, but it isn't true,
>> because 'NonTieredCompPolicy::initial_compile_level' returns
>> 'CompLevel_full_profile' when 'TIERED' is defined.
>>
>> On the one hand I want to be able to use any 'comp_level' in
>> 'CompileBroker::compile_method' from WhiteBoxAPI, so I need to remove
>> 'if (!TieredCompilation) { comp_level = CompLevel_highest_tier; }'. On
>> the other hand,  we must use only 'CompLevel_highest_tier' for
>> non-tiered in normal execution.
>>
>> The only place where used inaccurate 'comp_level' is
>> 'NonTieredCompPolicy::initial_compile_level', and I think it would be
>> better if it returns correct level.
>>
>> Best regards,
>> Igor Ignatyev
>>
>> On 04/04/2013 12:23 AM, Vladimir Kozlov wrote:
>>>  >>> I can add explicit checking for equality to 'CompLevel_any'.
>>>  > should i add this check?
>>>
>>> I don't think it is needed. May be comment.
>>>
>>> You convinced me about changes in CompilationPolicy::can_be_compiled().
>>>
>>> Now, why you need new changes in compilationPolicy.hpp? It was not in
>>> previous webrev. Yes, it may be correct but why?
>>>
>>> Vladimir
>>>
>>> On 4/3/13 12:35 PM, Igor Ignatyev wrote:
>>>>  >>> compileBroker.cpp - add assert() to check that.
>>>>  >> If I add it, WB.enqueueMethodForCompilation() will trigger this
>>>> assert,
>>>>  >> e.g. in test 'compiler/whitebox/EnqueueMethodForCompilationTest'.
>>>>  >
>>>>  > Which means the test introduces the case which never happens in
>>>> normal
>>>>  > execution. I am not sure it is good or bad.
>>>>  > You can relax assert with WhiteBoxAPI check. I really want this
>>>> assert
>>>>  > for normal execution.
>>>> i added assert and changed 'NonTieredCompPolicy::initial_compile_level'
>>>> since it return 'CompLevel_full_profile' instead of
>>>> 'CompLevel_full_optimization' for non-tiered C2 with Xcomp. sorry, i
>>>> missed this it due to poor preprocessor in my brain.
>>>>
>>>>>>> compilationPolicy.cpp - I don't think the change is correct since
>>>>>>> is_compile(CompLevel_any) returns false.
>>>>>> 'comp_level == CompLevel_any' is the same that 'comp_level ==
>>>>>> CompLevel_all' and this situation is processed in
>>>>>> compilationPolicy.cpp:124, so I think that it's correct, isn't it?
>>>>>> I can add explicit checking for equality to 'CompLevel_any'.
>>>> should i add this check?
>>>>
>>>>> You are right about CompLevel_any. What about next check in
>>>>> m->is_not_compilable():
>>>>>
>>>>>    if (is_method_handle_intrinsic())
>>>>>      return !is_synthetic();  // the generated adapters must be
>>>>> compiled
>>>>>
>>>> even if CompilationPolicy::can_be_compiled() return 'true' for invalid
>>>> comp_level, method won't be compiled since
>>>> CompileBroker::compiler(comp_level) will be 'NULL' in
>>>> CompileBroker::compile_method():
>>>>
>>>> AbstractCompiler *comp = CompileBroker::compiler(comp_level);
>>>> if (comp == NULL || !comp->can_compile_method(method) ||
>>>>      compilation_is_prohibited(method, osr_bci, comp_level)) {
>>>>    return NULL;
>>>> }
>>>>
>>>> updated webrev:
>>>> http://cr.openjdk.java.net/~iignatyev/8007288/webrev.02/
>>>>
>>>> Best regards,
>>>> Igor Ignatyev
>>>>
>>>> On 04/02/2013 11:19 PM, Vladimir Kozlov wrote:
>>>>> On 4/2/13 1:49 AM, Igor Ignatyev wrote:
>>>>>>  > compileBroker.cpp - add assert() to check that.
>>>>>> If I add it, WB.enqueueMethodForCompilation() will trigger this
>>>>>> assert,
>>>>>> e.g. in test 'compiler/whitebox/EnqueueMethodForCompilationTest'.
>>>>>
>>>>> Which means the test introduces the case which never happens in normal
>>>>> execution. I am not sure it is good or bad.
>>>>> You can relax assert with WhiteBoxAPI check. I really want this assert
>>>>> for normal execution.
>>>>>
>>>>>>
>>>>>>  > compilationPolicy.cpp - I don't think the change is correct since
>>>>>>  > is_compile(CompLevel_any) returns false.
>>>>>> 'comp_level == CompLevel_any' is the same that 'comp_level ==
>>>>>> CompLevel_all' and this situation is processed in
>>>>>> compilationPolicy.cpp:124, so I think that it's correct, isn't it?
>>>>>> I can add explicit checking for equality to 'CompLevel_any'.
>>>>>
>>>>> You are right about CompLevel_any. What about next check in
>>>>> m->is_not_compilable():
>>>>>
>>>>>    if (is_method_handle_intrinsic())
>>>>>      return !is_synthetic();  // the generated adapters must be
>>>>> compiled
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>>>
>>>>>>> Could you rename setForceInlineMethod() to
>>>>>>> testSetForceInlineMethod() to
>>>>>>> be more clear what it does?
>>>>>> I will rename setForceInlineMethod() and setDontInlineMethod().
>>>>>>
>>>>>> Best regards,
>>>>>> Igor Ignatyev
>>>>>>
>>>>>> On 04/01/2013 09:30 PM, Vladimir Kozlov wrote:
>>>>>>> compileBroker.cpp - add assert() to check that.
>>>>>>>
>>>>>>> compilationPolicy.cpp - I don't think the change is correct since
>>>>>>> is_compile(CompLevel_any) returns false.
>>>>>>>
>>>>>>> Could you rename setForceInlineMethod() to
>>>>>>> testSetForceInlineMethod() to
>>>>>>> be more clear what it does?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>>
>>>>>>> On 4/1/13 7:15 AM, Igor Ignatyev wrote:
>>>>>>>> test 'compiler/whitebox/EnqueueMethodForCompilationTest' has failed
>>>>>>>> during testing, because 'CompilationPolicy::can_be_compiled()'
>>>>>>>> returns
>>>>>>>> 'true' for invalid comp_level (less that -1 or great that 4).
>>>>>>>>
>>>>>>>> i added method 'is_compile' to check whether a value is valid
>>>>>>>> level of
>>>>>>>> compilation and rerun jprt.
>>>>>>>>
>>>>>>>> in new results:
>>>>>>>>    - compiler/whitebox/IsMethodCompilableTest failed on on
>>>>>>>> *-i586-*-c1
>>>>>>>> due to JDK-8007270
>>>>>>>>    - timeouts in other compiler's tests:
>>>>>>>>      * compiler/6663848/Tester
>>>>>>>>      * compiler/6711100/Test
>>>>>>>>      * compiler/6792161/Test6792161
>>>>>>>>      * compiler/6863420/Test
>>>>>>>>      * compiler/6863420/Test
>>>>>>>>      * compiler/6901572/Test
>>>>>>>>      * compiler/7100757/Test7100757
>>>>>>>>      * compiler/7119644/TestFloatVect
>>>>>>>>      * compiler/7196199/Test7196199
>>>>>>>>      * compiler/7199742/Test7199742
>>>>>>>>
>>>>>>>> updated webrev:
>>>>>>>> http://cr.openjdk.java.net/~iignatyev/8007288/webrev.01/
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Igor Ignatyev
>>>>>>>>
>>>>>>>> On 03/29/2013 01:52 AM, David Chase wrote:
>>>>>>>>> (Not a reviewer)
>>>>>>>>>
>>>>>>>>> Could you run these tests on the "east" JPRT queue and see if they
>>>>>>>>> have any problems there?
>>>>>>>>> We seem to have some surprisingly slow machines there, and that
>>>>>>>>> has
>>>>>>>>> caused problems with other tests.
>>>>>>>>>
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2013-03-28, at 5:41 PM, Igor Ignatyev
>>>>>>>>> <igor.ignatyev at oracle.com>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>> Please review the patch.
>>>>>>>>>>
>>>>>>>>>> 1. added new functions to WhiteBox:
>>>>>>>>>>   public native boolean setForceInlineMethod(Method method,
>>>>>>>>>> boolean
>>>>>>>>>> value);
>>>>>>>>>>   public native boolean enqueueMethodForCompilation(Method
>>>>>>>>>> method,
>>>>>>>>>> int compLevel);
>>>>>>>>>>   public native void    clearMethodState(Method method);
>>>>>>>>>> 2. related changes in not whitebox code:
>>>>>>>>>>   - added 'Method::clear_not_*_compilable()' and corresponded
>>>>>>>>>> 'AccessFlags::clear_not_*_compilable()'
>>>>>>>>>>   - added 'MethodData::init()' - reset method data into original
>>>>>>>>>> state. reset all counters, flags
>>>>>>>>>> 3. removed useless code:
>>>>>>>>>>   - 'if (!TieredCompilation) { comp_level =
>>>>>>>>>> CompLevel_highest_tier;
>>>>>>>>>> }' in CompileBroker::compile_method() -- there is no method
>>>>>>>>>> calls w/
>>>>>>>>>> comp_level != CompLevel_highest_tier in non-tiered
>>>>>>>>>>   - 'MethodData::initialize(methodHandle method)' -- declaration
>>>>>>>>>> w/o
>>>>>>>>>> definition
>>>>>>>>>> 4. added tests for new WB functions
>>>>>>>>>>
>>>>>>>>>> webrev: http://cr.openjdk.java.net/~iignatyev/8007288/webrev.00/
>>>>>>>>>> jbs: https://jbs.oracle.com/bugs/browse/JDK-8007288
>>>>>>>>>> --
>>>>>>>>>> Best regards,
>>>>>>>>>> Igor Ignatyev
>>>>>>>>>

More information about the hotspot-compiler-dev mailing list