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

[Vladimir Ivanov]

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"