RFR(M): 8007288: Additional WB API for compiler's testing
Igor Ignatyev
igor.ignatyev at oracle.com
Wed Apr 3 23:40:47 PDT 2013
>> 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