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

[Tom Rodriguez]

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