Request for reviews (S): 6880053

[Vladimir Kozlov]

Tom,

I forgot to add instance_id meet changes for TypeOopPtr
in the last webrev. I added it:

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

Thanks,
Vladimir

Vladimir Kozlov wrote:
> 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
>>>>>>>>>>>
>>>>>>
>>