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

[Tobias Hartmann]

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