RFR(M): 8007288: Additional WB API for compiler's testing
Igor Ignatyev
igor.ignatyev at oracle.com
Fri Apr 5 00:31:43 PDT 2013
Vladimir,
Thanks for the review.
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