Request for reviews (S): 6880053

[Tom Rodriguez]

At one point in the original discussion you suggested using  
MembarCPUOrder to enforce the ordering which allows us to leave the  
CheckCastPP in it's original location without adding another  
CheckCastPP.  I'd prefer that to playing weird games with the types.   
Is that a viable fix?

tom

On Sep 9, 2009, at 5:32 PM, Vladimir Kozlov wrote:

> It is valid C2 type since all type methods (xmeet) process it
> but it should not exist from java point of view. And I'm using it
> with assumption that none java programs use such type.
>
> I am also not fully comfortable with it but I can't find
> other type which could be used (I tried).
> I talked with John and he agreed with this change.
>
> I ran CTW and it did not find problems with this change.
> But we may still hit some problems where we would expect
> a real java type and will get this one. But such cases
> should be caught by asserts we have.
>
> The other solution (which we rejected before) is to modify
> oop map build to list raw pointers which have CheckCastPP
> users. Then we will not need the second CheckCastPP.
>
> Vladimir
>
> Tom Rodriguez wrote:
>> I suspected that was the purpose but TypeOopPtr::NOTNULL seems  
>> nonsensical to me.  Is that really a valid type?
>> tom
>> On Sep 9, 2009, at 4:43 PM, Vladimir Kozlov wrote:
>>> As you remember we have 2 CheckCastPP nodes in clone() intrinsic.
>>> One is original which we move and pin below arraycopy call
>>> to guaranty that all memory accesses to the cloned object
>>> are placed below arraycopy.
>>> And the second is used to have a live oop across arraycopy call.
>>> If both have the same type (bug's case) the  
>>> CheckCastPPNode::identity()
>>> will eliminate the original CheckCastPP since it references the  
>>> second:
>>>
>>> // Replace raw memory edge with new CheckCastPP to have a live oop
>>> // at safepoints instead of raw value.
>>> assert(new_obj->is_CheckCastPP() && new_obj->in(1) == alloc_obj- 
>>> >in(1), "sanity");
>>> alloc_obj->set_req(1, new_obj);
>>>
>>> Vladimir
>>>
>>> Tom Rodriguez wrote:
>>>> I'm don't understand this.  What is TypeOopPtr::NOTNULL supposed  
>>>> to represent?  That doesn't seem like a valid type in the lattice  
>>>> since it would have to correspond to Object:NotNull which is  
>>>> already represented by TypeInstPtr::NOTNULL.  Why will the graph  
>>>> be incorrect if the type is TypeInstPtr::NOTNULL?
>>>> tom
>>>> On Sep 9, 2009, at 3:35 PM, Vladimir Kozlov wrote:
>>>>> http://cr.openjdk.java.net/~kvn/6880053/webrev.00
>>>>>
>>>>> Fixed 6880053: assert(alloc_obj->as_CheckCastPP()->type() !=  
>>>>> TypeInstPtr::NOTNULL)
>>>>>
>>>>> Problem:
>>>>> I added this assert in 6875577 fix to catch cases when
>>>>> the type of the cloned object is TypeInstPtr::NOTNULL
>>>>> since it will produce incorrect graph.
>>>>> And Nightly testing found such case: the method
>>>>> sun.reflect.GeneratedMethodAccessor4.invoke(Object o, Object[] ao)
>>>>> returns the clone of the Object o argument.
>>>>>
>>>>> Solution:
>>>>> Use more general oop type TypeOopPtr::NOTNULL for
>>>>> additional CheckCastPP node in clone() instrinsic and arraycopy.
>>>>> Also fix instance_id meet for TypeOopPtr.
>>>>>
>>>>> Reviewed by:
>>>>>
>>>>> Fix verified (y/n): y, test
>>>>>
>>>>> Other testing:
>>>>> JPRT
>>>>>