Request for reviews (S): 6880053

Vladimir Kozlov Vladimir.Kozlov at Sun.COM
Wed Sep 9 19:28:02 PDT 2009

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.


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

More information about the hotspot-compiler-dev mailing list