Request for reviews (S): 6823453: DeoptimizeALot causes fastdebug server jvm to fail with assert(false,"unscheduable graph")

[Tom Rodriguez]

Just like the earlier fix isn't this just delaying the problem?   
Couldn't a more complicated example lead to the constant not being  
visible at parse time but resulting in the exact same graph shape that  
caused this failure?  Is there some problem with the idea of  
correcting the fallthrough control of the allocation itself?

tom

On Apr 2, 2009, at 10:54 AM, Vladimir Kozlov wrote:

> Here are changes which implement the uncommon trap for
> unreachable path when array length is negative.
>
> http://cr.openjdk.java.net/~kvn/6823453/webrev.02
>
> Thanks,
> Vladimir
>
> Vladimir Kozlov wrote:
>> Generating uncommon trap during parsing when length < 0 works:
>> @@ -3125,13 +3126,26 @@ Node* GraphKit::new_array(Node* klass_no
>>   Node* javaoop = set_output_for_allocation(alloc, ary_type,  
>> raw_mem_only);
>> +    Node* ccast = alloc->make_ideal_length(ary_type, &_gvn);
>> +    if (ccast != length && length_type->_hi < 0) {
>> +      uncommon_trap(Deoptimization::Reason_unreached,
>> +                    Deoptimization::Action_reinterpret,
>> +                    NULL, "negative length", true);
>> +    }
>> +
>>   // Cast length on remaining path to be as narrow as possible
>>   if (map()->find_edge(length) >= 0) {
>>     Node* ccast = alloc->make_ideal_length(ary_type, &_gvn);
>> Vladimir
>> Tom Rodriguez wrote:
>>>
>>> On Apr 1, 2009, at 2:50 PM, Vladimir Kozlov wrote:
>>>
>>>> You are absolutely right.
>>>> The problem is the current code replace data path with TOP
>>>> after the full igvn but control path stays alive.
>>>> My changes remove CastII if it can produce TOP so the data path
>>>> stays alive also.
>>>>
>>>> I like your suggestion to cut the control path during parsing but
>>>> it is against our rule which is to let IGVN do cutting
>>>> since during parsing graph is not complete.
>>>
>>> I wasn't suggesting that we do this during parse.  It should be  
>>> part of AllocateArrayNode::Ideal when can_reshape is true.
>>>
>>> tom
>>>
>>>>
>>>>
>>>> Vladimir
>>>>
>>>> Tom Rodriguez wrote:
>>>>> Isn't this just delaying the problem?  If you hide the negative  
>>>>> value behind something that won't get simplified until the full  
>>>>> igvn pass then you're back where you started, aren't you?  Or is  
>>>>> this something that would only be a problem during parse?
>>>>
>>>>
>>>>
>>>>> The root of the problem is that the negative length effectively  
>>>>> proves that the control flow following the allocation is all  
>>>>> dead since it will throw an exception but our graph doesn't  
>>>>> express this.  The allocation stays around and simply becomes a  
>>>>> slow path call that will throw the exception but any control  
>>>>> flow that follows it is actually dead but can't fold up.  Maybe  
>>>>> we need to hammer the fall through projection of the  
>>>>> AllocateArrayNode to be a HaltNode to indicate that the fall  
>>>>> through path is unreachable?
>>>>> tom
>>>>> On Apr 1, 2009, at 11:57 AM, Vladimir Kozlov wrote:
>>>>>> I added test case suggested by John.
>>>>>> I also replaced the code in GraphKit::new_array
>>>>>> with the _gvn.transform(ccast) call and the result's
>>>>>> type check as John suggested. I think it is safe
>>>>>> to not delay the transform() call since the TOP result
>>>>>> will not be used.
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>>
>>>>>> http://cr.openjdk.java.net/~kvn/6823453/webrev.01
>>>>>>
>>>>>> Fixed 6823453: DeoptimizeALot causes fastdebug server jvm to  
>>>>>> fail with assert(false,"unscheduable graph")
>>>>>>
>>>>>> Problem:
>>>>>> The code in GraphKit::new_array replaces a negative value with
>>>>>> TOP (CastII(-1)#0) for all uses if it is used as new array  
>>>>>> length.
>>>>>> Incorrect ideal graph is generated as result.
>>>>>>
>>>>>> Solution:
>>>>>> Don't replace a value with TOP.
>>>>>> Added regression test.
>>>>>>
>>>>>> Reviewed by:
>>>>>>
>>>>>> Fix verified (y/n): y, bug tests
>>>>>>
>>>>>> Other testing:
>>>>>> JPRT
>>>>>>
>>>