Request for reviews (S): 6880053

[Tom Rodriguez]

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
>>>>>>>>>
>>>>