Request for reviews (S): 6880053

[Vladimir Kozlov]

You are right. It doesn't need to be optional.
The code is in repository already, I pushed it on Friday for testing over weekend.
So I will change it next time.

Thanks,
Vladimir

Tom Rodriguez wrote:
> Why not require instance_id to be specified instead of it being optional?
> 
> tom
> 
> On Sep 10, 2009, at 6:08 PM, Vladimir Kozlov wrote:
> 
>> 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
>>>>>>>>>>>>>
>>>>>>>>
>>>>
>