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

[Tobias Hartmann]

Thanks, Vladimir!

Best,
Tobias

On 05.11.2015 15:09, Vladimir Kozlov wrote:
>> New webrev:
>> http://cr.openjdk.java.net/~thartmann/8140574/webrev.05/
> 
> Very nice.
> 
> Thanks,
> Vladimir
> 
> On 11/5/15 6:50 PM, Tobias Hartmann wrote:
>> Hi Roland,
>>
>> On 04.11.2015 10:18, Roland Westrelin wrote:
>>>>> I think you need to check that should_reexecute() is true for the JVMState of the dominating test.
>>>>
>>>> I added the check. Should we then remove the filter for "Reason_unstable_if" and handle all types of uncommon traps as long as they have the re-execute flag set?
>>>
>>> Yes, I think you can remove that check.
>>
>> Okay, done.
>>
>>>>> I don’t know if that part is good or not:
>>>>>
>>>>> 2911   JsrSet* jsr_null = new ciTypeFlow::JsrSet(NULL);
>>>>> 2914   Block*     block = get_block_for(index, jsr_null, ciTypeFlow::no_create);
>>>>> 2915   Block* dom_block = get_block_for(dom_index, jsr_null, ciTypeFlow::no_create);
>>>>>
>>>>> (The use of JsrSet null that I don’t understand)
>>>>
>>>> I think the JsrSet is used during data flow analysis to record all the active jump to subroutine (jsr) instructions with destination and return address.
>>>>
>>>> // During abstract interpretation, JsrSets are used to determine
>>>> // whether two paths which reach a given block are unique, and
>>>> // should be cloned apart, or are compatible, and should merge
>>>> // together.
>>>>
>>>> I assumed that for our purpose of checking domination this does not matter and therefore used an empty JsrSet as it's done in ciTypeFlow::flow_types() and ciTypeFlow::get_start_state(). JsrSet::is_compatible_with(JsrSet* other) returns true if 'other' is empty.
>>>>
>>>> Do you think we have to check domination for the blocks corresponding to all possible JsrSets at this bci?
>>>
>>> I’m still unsure this is correct in the presence of jsrs. What if we reach a block from multiple jsrs? Wouldn’t there be cases where they are in different jsr sets?
>>
>> Yes, that's what I meant with we would "have to check domination for the blocks corresponding to all possible JsrSets at this bci".
>>
>>> jsrs are uncommon enough AFAIU that I would bail out of this optimization if the method has any (ciMethod::has_jsrs()).
>>
>> I agree. The JVM instruction set specification [1] says that "jsr" was only used "prior to Java SE 6". So most likely this bytecode is quite rare.
>>
>> I added a check and the corresponding assert to is_dominated_by().
>>
>>>>> Also, the same bci can be in multiple blocks because ciTypeFlow clones some block (ciTypeFlow::clone_loop_head()) and I wonder if that can be a problem or not.
>>>>
>>>> We get the block via ciTypeFlow::get_block_for() which checks block->is_backedge_copy() and therefore should only return the "original" block and no clones (clones are marked by get_block_for() if invoked with 'create_backedge_copy'). So I don't think this is a problem.
>>>
>>> I think that’s good then.
>>>
>>> ciTypeFlow::clone_loop_head() updates successor blocks so you need to update predecessors too.
>>
>> Right, done.
>>
>>> Also, what about exception edges for predecessors? I think you need to take them into account too.
>>
>> Yes, we need to account for them as well. I added the exception edges to the predecessor list.
>>
>>> I also think you need to check that the jvms stacks of method/bci are identical for both uncommon traps except for the top bci to make sure we got to the method from the same call stack.
>>
>> Good catch. I'm now using JVMState::same_calls_as() to verify that the callers of each JVMState have the same stack.
>>
>> New webrev:
>> http://cr.openjdk.java.net/~thartmann/8140574/webrev.05/
>>
>> I verified that the checks in TestExplicitRangeChecks still fold.
>>
>> Thanks,
>> Tobias
>>
>> [1] https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-6.html#jvms-6.5.jsr
>>