Request reviews (S): 6896352: CTW fails hotspot/src/share/vm/opto/escape.cpp:1155

[Vladimir Kozlov]

Thanks, Tom

Vladimir

Tom Rodriguez wrote:
> 
> On Nov 4, 2009, at 12:10 PM, Vladimir Kozlov wrote:
> 
>> Yes, it works currently without volatile because of side effect
>> but I did not feel comfortable with the code as it looks.
>> May be I should just add the comment that C++ will not remove
>> the call since it has side effect.
> 
> It seems ok without the volatile to me but add a comment if you like.
> 
>>
>> The code in escape.cpp creates new alias type with instance id
>> which is different from original general type created now in
>> Ideal_common (and in different places before).
> 
> I'd forgotten about the instance_id.  That makes sense then.
> 
> tom
> 
>> When we process allocations candidates for scalar replacement
>> we cast their type (type of CheckCastPP node) to instance id:
>>
>>      const TypeOopPtr *t = igvn->type(n)->isa_oopptr();
>>      if (t == NULL)
>>        continue;  // not a TypeInstPtr
>>      tinst = 
>> t->cast_to_exactness(true)->is_oopptr()->cast_to_instance_id(ni);
>>
>> Than we pass the allocation (CheckCastPP node) node as base to
>> split_AddP() and replace AddP node's type with new one:
>>
>>  const TypeOopPtr *base_t = igvn->type(base)->isa_oopptr();
>> ...
>>  const TypeOopPtr *tinst = base_t->add_offset(t->offset())->is_oopptr();
>>  // Do NOT remove the next line: ensure a new alias index is allocated
>>  // for the instance type
>>  int alias_idx = _compile->get_alias_index(tinst);
>>  igvn->set_type(addp, tinst);
>>
>> This is the core for scalar replacement and how we separate memory
>> slices for non-escaping objects which we intend to eliminate.
>>
>> Vladimir
>>
>> Tom Rodriguez wrote:
>>> I don't think adding volatile could have the effect you are hoping 
>>> for and I also don't think it's needed even if it did.  
>>> get_alias_index has a side effect over in find_alias_type so it's 
>>> impossible for it to be optimized away even if the return value isn't 
>>> used.  Also if the new call in Ideal_common is doing it's job then do 
>>> you really need the call in escape.cpp?  I don't see how splitting an 
>>> AddP could create a new alias type if one already existed for that AddP.
>>> tom
>>> On Nov 4, 2009, at 10:51 AM, Vladimir Kozlov wrote:
>>>>
>>>> http://cr.openjdk.java.net/~kvn/6896352/webrev.00
>>>>
>>>> Fixed 6896352: CTW fails hotspot/src/share/vm/opto/escape.cpp:1155
>>>>
>>>> Problem:
>>>> Alias type for LoadUS(ConP char[]) node from the test
>>>> was not defined during Parse before EA since
>>>> C->get_alias_index(phase->type(address)) was not called.
>>>> But EA expects all alias types to be defined before it starts.
>>>>
>>>> Solution:
>>>> Always call C->get_alias_index(phase->type(address)) in
>>>> MemNode::Ideal_common() which is called by all memory nodes.
>>>> I added "volatile" to expressions to avoid C++ removal
>>>> since the result is not used.
>>>>
>>>> Reviewed by:
>>>>
>>>> Fix verified (y/n): y, test
>>>>
>>>> Other testing:
>>>> JPRT, CTW
>>>>
>