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

[Vladimir Kozlov]

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
>>>>>
>>