review (S) for 6915557: assert(_gvn.type(l)->higher_equal(type), "must constrain OSR typestate") with debug build

[Vladimir Kozlov]

Perfect :)

Vladimir

Tom Rodriguez wrote:
> I changed the comment to this:
> 
>       // In our current system it's illegal for jsr addresses to be                                                                                         
>       // live into an OSR entry point because the compiler performs                                                                                         
>       // inlining of jsrs.  ciTypeFlow has a bailout that detect this                                                                                       
>       // case and aborts the compile if addresses are live into an OSR                                                                                     
>       // entry point.  Because of that we can assume that any address                                                                                       
>       // locals at the OSR entry point are dead.  Method liveness                                                                                           
>       // isn't precise enought to figure out that they are dead in all                                                                                     
>       // cases so simply skip checking address locals all                                                                                                   
>       // together. Any type check is guaranteed to fail since the                                                                                           
>       // interpreter type is the result of a load which might have any                                                                                     
>       // value and the expected type is a constant. 
> 
> tom
> 
> On Feb 24, 2010, at 10:37 PM, Vladimir Kozlov wrote:
> 
>> It would be nice if you do since it is difficult to understand the problem from the current comment.
>> At least add what you said in the request:
>>
>> If the OSR entry point has any live address we will fail this test but
>> the responsibility for making sure that we don't actually have a live
>> jsr during OSR entry is managed by ciTypeFlow itself so it should be
>> safe to simply skip the check.
>>
>> Thanks,
>> Vladimir
>>
>> Tom Rodriguez wrote:
>>> Do you think I should rewrite/expand the comment?
>>> tom
>>> On Feb 24, 2010, at 10:11 PM, Vladimir Kozlov wrote:
>>>> Thank you, Tom, for explanation.
>>>> Changes are good.
>>>>
>>>> Vladimir
>>>>
>>>> Tom Rodriguez wrote:
>>>>> On Feb 24, 2010, at 6:13 PM, Vladimir Kozlov wrote:
>>>>>> Tom,
>>>>>>
>>>>>> So it is safe to skip the type check at this place because
>>>>>> in an other place (in ciTypeFlow) we already have such check?
>>>>>>
>>>>>> Your new comment says that we don't do type check because it will fail anyway
>>>>>> but it does not explain why it is safe to skip it at that place.
>>>>> In our current system it's illegal to compile an OSR with a live jsr and it's ciTypeFlows responsibility to figure that out.  See ciTypeFlow::JsrSet::apply_control.  So any address in the OSR entry state must be invalid.  A precise version of method liveness or a more complicated version of ciTypeFlow, would be able to prove that but since ours can't prove this we have to ignore them ourselves.  This is something we've always been exposed to but it happened in the past that the copy of the OSR block that we chose to use as the entry had those locals as bottom instead of containing an address.  Because of cloning caused by jsr/ret that are hundreds of copies of that block, some which say that local is bottom and the others all say it's some constant.  If we chose a one with a constant address then we will hit this assert.
>>>>> tom
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> Tom Rodriguez wrote:
>>>>>>> http://cr.openjdk.java.net/~never/6915557
>>>>>>> 6915557: assert(_gvn.type(l)->higher_equal(type),"must constrain OSR typestate") with debug build
>>>>>>> Reviewed-by:
>>>>>>> The fix for 6892079 to eliminate asserts about address types in OSR
>>>>>>> was insufficient because sometimes method liveness may consider locals
>>>>>>> to live that actually aren't because of the conservativeness of its
>>>>>>> analysis.  I think the fix is simply to not check address types.  If
>>>>>>> the OSR entry point has any live address we will fail this test but
>>>>>>> the resonsibility for making sure that we don't actually have a live
>>>>>>> jsr during OSR entry is managed by ciTypeFlow itself so it should be
>>>>>>> safe to simply skip the check.  Tested with failing test case.
>>>>>>> src/share/vm/opto/parse1.cpp
>