[9] RFR(S): 8140574: C2 must re-execute checks after deoptimizing from merged uncommon traps

[Tobias Hartmann]

Thanks for the review, Vladimir and Roland!

On 27.10.2015 17:17, Roland Westrelin wrote:
>> I see. So the only pattern we are looking for is where 'throw' is replaced with unstable_if uncommon trap.
> 
> Actually, thinking about this more, I don’t think testing for unstable_if is sufficient. We could have something like this:
> 
> if (i < 0) {
>   if (some_condition) {
>     // never taken so unc unstable_if
>   }
> }

I agree that this would be a problem but why would be add an uncommon trap here? Either the i<0 branch is never executed, then we would add the uncommon trap like this

 if (i < 0) {
   // unc unstable_if
 }

or it was executed and then also "some_condition" was true and we would not add an uncommon trap at all.

> if (i >= length) {
>   // never taken so unc unstable_if
> }
> 
> Then if the compiler proves some_condition is always true, we have:
> 
> if (i < 0) {
>   // never taken so unc unstable_if
> }
> if (i >= length) {
>   // never taken so unc unstable_if
> }
> 
> and we go ahead with the folding but the deoptimizatin in the first branch would have us resume execution after the i < 0 test.
> 
> So I think we have to restrict when we apply that transformation even more, maybe matching the expected bytecode pattern more closely?

I'm not sure how to do this. We would somehow have to make sure that the dominating uncommon trap "belongs" to the dominating if and not to some other if that was removed, right?

Thanks,
Tobias


> 
> Roland.
> 
>>
>> Okay then.
>>
>> Thanks,
>> Vladimir
>>
>> On 10/27/15 11:09 PM, Roland Westrelin wrote:
>>>> Should we just additionally check re-execute flag?
>>>>
>>>> dom_unc->jvms()->should_reexecute()
>>>>
>>>> Unfortunately wee can't change reexecute flag since JVM state is already recorded according to it.
>>>>
>>>> Roland, what do you think?
>>>
>>> Is checking for should_reexecute() sufficient? Couldn’t we reexecute some bytecode in one branch that caused the branch to become dead but not the if itself?
>>>
>>> This change was put in so we optimize the pattern:
>>>
>>> if (i < 0 || i >= length) { throw… }
>>>
>>> of explicit array bound checks. Restricting it to unstable_if uncommon traps, doesn’t break that scenario so I would say that fix is good enough and feels safe.
>>>
>>> Roland.
>>>
>