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

[Vladimir Kozlov]

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"