Request for reviews (S): 6880053

[Vladimir Kozlov]

I used MembarCPUOrder, removed second CheckCastPP and
ran through CTW, JPRT and refworkload.
I don't see performance difference.

http://cr.openjdk.java.net/~kvn/6880053/webrev.01

Vladimir

Vladimir Kozlov wrote:
> I am taking my previous statement back.
> 
> We are already generating MembarCPUOrder after generic arraycopy
> under flag InsertMemBarAfterArraycopy which is true by default.
> The only place where we miss it is in clone() intrinsic.
> I will check the performance with it. Also we may need to
> generate it always (not under flag) when it is used for
> zeroing reduction.
> 
> Vladimir
> 
> Vladimir Kozlov wrote:
>> The problem with membar is that it is barrier for all memory,
>> we can't set it to slices corresponding only to this object.
>> It will prevent memory optimizations for other objects.
>> I don't remember other problems then this performance concern.
>>
>> Vladimir
>>
>> Tom Rodriguez wrote:
>>> 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
>>>>>>>>
>>>