review (XS) for 6982533: Crash in ~StubRoutines::jbyte_fill with AggressiveOpts enabled

[Tom Rodriguez]

On Sep 7, 2010, at 11:22 AM, Vladimir Kozlov wrote:

> Tom,
> 
> I am fine with your index explicit check, I only want an additional check for array ptr and your suggestion is good.

Thanks!

tom

> 
> Your current changes look good.
> 
> Thanks,
> Vladimir
> 
> Tom Rodriguez wrote:
>> On Sep 7, 2010, at 10:49 AM, Vladimir Kozlov wrote:
>>> Looks good but could we also check (in assert?) that address type is array pointer?
>> I considered adding that as an early check but I wanted an explicit check that we found the index to confirm the shape of the address expression.  I could add an early weed out test:
>> --- a/src/share/vm/opto/loopTransform.cpp       Fri Sep 03 13:31:03 2010 -0700                                                                       +++ b/src/share/vm/opto/loopTransform.cpp       Tue Sep 07 11:04:07 2010 -0700                                                                       @@ -2417,6 +2417,8 @@ bool PhaseIdealLoop::match_fill_loop(Ide
>>       Node* value = n->in(MemNode::ValueIn);                                                                                                                if (!lpt->is_invariant(value)) {                                                                                                                        msg  = "variant store value";                                                                                                               +      } else if (!_igvn.type(n->in(MemNode::Address))->isa_aryptr()) {                                                                               +        msg = "not array address";                                                                                                                          }                                                                                                                                                    store = n;                                                                                                                                            store_value = value; tom
>>> Thanks,
>>> Vladimir
>>> 
>>> Tom Rodriguez wrote:
>>>> http://cr.openjdk.java.net/~never/6982533
>>>> 6982533: Crash in  ~StubRoutines::jbyte_fill with AggressiveOpts enabled
>>>> Reviewed-by:
>>>> The logic for matching a byte fill is missing a check for the use of
>>>> the index.  It normally happens as part of the check for a shift
>>>> expression but since a byte array doesn't have a shift the check is
>>>> missed.  The fix is to make the index check explicit.  The reason it
>>>> didn't always crash was because of differences in heap size caused by
>>>> ergonomics.  With a 16m heap it crashes on any machine.  Tested with
>>>> failing test case.
>>>> src/share/vm/opto/loopTransform.cpp