Request for reviews (S): 6880053

[Vladimir Kozlov]

Thanks Tom

Vladimir

Tom Rodriguez wrote:
> I like that much better.
> 
> tom
> 
> On Sep 10, 2009, at 2:35 PM, Vladimir Kozlov wrote:
> 
>> 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
>>>>>>>>>>
>>>>>
>