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

Vladimir Kozlov vladimir.kozlov at oracle.com
Fri Apr 5 13:30:23 PDT 2013 

Yes, I am okay with it.

Vladimir

On 4/5/13 12:50 PM, Igor Ignatyev wrote:
> Vladimir,
>
> Just want to check that I understand correctly: you're ok with pushing
> webrev.02[*], aren't you?
>
> [*] http://cr.openjdk.java.net/~iignatyev/8007288/webrev.02/
>
> Best regards,
> Igor Ignatyev
>
> On 04/05/2013 05:03 AM, Vladimir Kozlov wrote:
>> 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