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

Igor Ignatyev igor.ignatyev at oracle.com
Thu Apr 4 10:13:31 PDT 2013 

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