Regarding 8033626: C2: Merging exception state: reexecute=true vs reexecute=false

[Vladimir Ivanov]

I played with uncommon trap approach and it works fine.

While I'm finishing extensive testing, I'd be grateful for early feedback.

Preliminary fix:
http://cr.openjdk.java.net/~vlivanov/8033626/webrev.00/

The fix is to bail out to interpreter if any exception is generated in 
new_instance_C. There's no need to update exception state. When 
reexecuting the call, native version of Object::clone (JVM_Clone) will 
be invoked.

Controlling exception handling explicitly by passing 
deoptimize_on_exception parameters looks cleaner to me, than having a 
check (is_LibraryCallKit() or JVMS reexecute bit) burden in 
GraphKit::make_slow_call_ex.

I fixed only Object::clone case, since I don't see other places where 
similar IR shape can be produced.

Best regards,
Vladimir Ivanov

On 6/3/14 2:27 AM, Vladimir Kozlov wrote:
> Unfortunately it may not be a whole solution. The uncommon trap may
> still needs to be recorded as an exceptions handler for compiled code so
> that when we return from new_instance_C() runtime call with OOM
> exception it will find it. See GraphKit::gen_stub(),
> generate_catch_exception() and
> SharedRuntime::raw_exception_handler_for_return_address()
>
> Also do additional tests with StressCompiledExceptionHandlers otherwise
> we deoptimize immediately on the return from new_instance_C().
>
> In short, please investigate all return paths from new_instance_C() when
> there is pending OOM exception and we have uncommon trap instead of
> CreateExNode.
>
> thanks,
> Vladimir
>
> On 6/2/14 12:21 PM, Vladimir Ivanov wrote:
>> Yes, uncommon trap should solve the problem.
>> Thanks for excellent idea!
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> On 5/31/14 6:30 AM, Vladimir Kozlov wrote:
>>> I thought about this more. I think we should not generate exceptions in
>>> intrinsic when we are going to re-execute the method. Interpreter will
>>> throw exception if needed and intrinsic has wrong bci anyway (bci of
>>> call site and not allocation). *We should generate plain "intrinsic"
>>> uncommon trap in such case*.
>>> There are GraphKit::is_LibraryCallKit() and is_Parse() methods which
>>> allow to check where the request for exception generation is coming
>>> from.
>>>
>>> Regards,
>>> Vladimir
>>>
>>> On 5/29/14 7:00 PM, Vladimir Kozlov wrote:
>>>> Now I understand the problem. I think we need to keep separate an
>>>> exception code which requires re-execution. Currently
>>>> we don't check for should_reexecute() when we merge or search for
>>>> existing exception code (we only compare type).
>>>>
>>>> Vladimir K.
>>>>
>>>> On 5/29/14 2:14 AM, Vladimir Ivanov wrote:
>>>>> The problem is with exception state of two branches [1].
>>>>> Exception on fast path comes from GraphKit::new_instance during
>>>>> intrinsic construction in LibraryCallKit::inline_native_clone. It is
>>>>> executed under should_reexecute=true. PreserveReexecuteState doesn't
>>>>> touch exception state, so kit.add_exception_states_from(new_jvms) (@
>>>>> callGenerator.cpp:703) tries to accumulate incompatible exceptions
>>>>> from
>>>>> fast & slow paths.
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>>
>>>>> [1] FAST PATH (intrinsic):
>>>>>
>>>>> JVMS depth=1 loc=5 stk=9 arg=10 mon=14 scalar=14 end=14 mondepth=0
>>>>> sp=1
>>>>> bci=12 reexecute=false method=virtual jobject
>>>>>
>>>>> Exception state:
>>>>>
>>>>> JVMS depth=1 loc=5 stk=9 arg=9 mon=14 scalar=14 end=14 mondepth=0 sp=0
>>>>> bci=12 reexecute=true method=virtual jobject
>>>>>
>>>>> SLOW PATH
>>>>>
>>>>> JVMS depth=1 loc=5 stk=9 arg=10 mon=14 scalar=14 end=14 mondepth=0
>>>>> sp=1
>>>>> bci=12 reexecute=false method=virtual jobject
>>>>>
>>>>> Exception state:
>>>>>
>>>>> JVMS depth=1 loc=5 stk=9 arg=9 mon=14 scalar=14 end=14 mondepth=0 sp=0
>>>>> bci=12 reexecute=false method=virtual jobject
>>>>>
>>>>>
>>>>> On 5/29/14 2:18 AM, Vladimir Kozlov wrote:
>>>>>> PredictedIntrinsicGenerator was added to have a general predicate.
>>>>>> For
>>>>>> example, AES crypto predicate checks klass of an object pointed by a
>>>>>> field. PredictedCallGenerator checks klass of method's holder only.
>>>>>>
>>>>>> Both cases would have the same problem with reexecute. Using
>>>>>> PredictedIntrinsicGenerator will not help you.
>>>>>>
>>>>>> But you should not have this problem at the merge point, because
>>>>>> reexecute should be reset back after intrinsic is generated:
>>>>>>
>>>>>>    { PreserveReexecuteState preexecs(this);
>>>>>>      jvms()->set_should_reexecute(true);
>>>>>>
>>>>>> It is strange that it is still true after PreserveReexecuteState
>>>>>> destructor.
>>>>>>
>>>>>> Vladimir K
>>>>>>
>>>>>> On 5/28/14 9:37 AM, Vladimir Ivanov wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> I'm looking at 8033626 [1]:
>>>>>>> "assert(ex_map->jvms()->same_calls_as(_exceptions->jvms())) failed:
>>>>>>> all
>>>>>>> collected exceptions must come from the same place".
>>>>>>>
>>>>>>> 8014447 [2] introduced new code shape for virtual intrinsifiable
>>>>>>> methods
>>>>>>> (Object::hashCode & Object::clone): "invokevirtual Object.clone()"
>>>>>>> can
>>>>>>> be represented as a PredictedCallGenerator with LibraryIntrinsic on
>>>>>>> fast
>>>>>>> path and VirtualCallGenerator on slow path.
>>>>>>>
>>>>>>> The problem arises for Object::clone, because for OOM case
>>>>>>> intrinsified
>>>>>>> version requires reexecution (reexecute=true), but slow path
>>>>>>> (virtual
>>>>>>> dispatch) doesn't (reexecute=false). Further attempt to finish the
>>>>>>> diamond and merge 2 exception states fires the assertion.
>>>>>>>
>>>>>>> So, I have 2 questions:
>>>>>>>    (1) I don't see any way to merge reexecute=true & reexecute=false
>>>>>>> states, so both branches should have the same reexecute status. Am I
>>>>>>> missing something?
>>>>>>>
>>>>>>>    (2) Does PredictedCallGenerator actually support such shape
>>>>>>> (intrinsic on fast path)? There are 2 call generators:
>>>>>>> PredictedIntrinsicGenerator & PredictedCallGenerator. What's the
>>>>>>> difference?
>>>>>>>
>>>>>>> I see there's a way to customize predicate in
>>>>>>> LibraryCallKit::try_to_predicate() for PredictedIntrinsicGenerator,
>>>>>>> but
>>>>>>> is it the only reason why PredictedIntrinsicGenerator was
>>>>>>> introduced?
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Vladimir Ivanov
>>>>>>>
>>>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8033626
>>>>>>>
>>>>>>> [2] https://bugs.openjdk.java.net/browse/JDK-8014447
>>>>>>>      "Object.hashCode intrinsic breaks inline caches"