review (S) for 6856025: assert(_base >= OopPtr && _base <= KlassPtr,"Not a Java pointer")

[Vladimir Kozlov]

Sorry, in the original webrev (webrev/g1) which you sent me
there were no changes in graphKit.cpp so I assumed you missed it.
But I see now in the final changes you have it.

Sorry for confusion.

Thanks,
Vladimir

Tom Rodriguez wrote:
> 
> On Jun 30, 2009, at 2:49 PM, Vladimir Kozlov wrote:
> 
>> Tom Rodriguez wrote:
>>> On Jun 30, 2009, at 2:33 PM, Vladimir Kozlov wrote:
>>>> After sending this I don't see that you fixed store_oop_to_unknown.
>>>> Is it intentional to use TypeInstPtr::BOTTOM as val_type in case of 
>>>> narrow field/element?
>>> The value_type for the prebarrier is declared to be TypeOopPtr since 
>>> it bottoms out in LoadNode::make which always returns the wide type.  
>>> So TypeInstPtr::BOTTOM is the worst case type for the 
>>> store_oop_to_unknown case.  Is there something wrong with that?
>>
>> Nothing wrong with it. I'm just wondering why you don't use more precise
>> type for array's element (instance's field type is fine) by using 
>> make_oopptr().
> 
> I'm confused.  This whole chunk tries to pick the best type it can for 
> store_oop_to_unknown.
> 
>   const TypeOopPtr* val_type = NULL;
>   if (adr_type->isa_instptr()) {
>     if (at->field() != NULL) {
>       // known field.  This code is a copy of the do_put_xxx logic.
>       ciField* field = at->field();
>       if (!field->type()->is_loaded()) {
>         val_type = TypeInstPtr::BOTTOM;
>       } else {
>         val_type = TypeOopPtr::make_from_klass(field->type()->as_klass());
>       }
>     }
>   } else if (adr_type->isa_aryptr()) {
>     val_type = adr_type->is_aryptr()->elem()->make_oopptr();
>   }
>   if (val_type == NULL) {
>     val_type = TypeInstPtr::BOTTOM;
>   }
> 
> What change are you suggesting?
> 
> tom
> 
>>
>> Vladimir
>>
>>> tom
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> Vladimir Kozlov wrote:
>>>>> Looks good.
>>>>> Vladimir
>>>>> Tom Rodriguez wrote:
>>>>>> http://cr.openjdk.java.net/~never/6856025/
>