Request reviews (L): 6895383: JCK test throws NPE for method compiled with Escape Analysis

[Vladimir Kozlov]

http://cr.openjdk.java.net/~kvn/6895383/webrev.06

Move ClearArray logic into separate method ClearArray::step_through()
and added ClearArray to NodeClasses to use is_ClearArray() instead of Opcode().

Added asserts to check cases which don't have to be handled.

I also changed instance type setting for sequential CheckCastPP nodes
to change following CheckCastPP node type to allocation type
if it is subtype. For that I replaced the next check:

tinst->cast_to_instance_id(TypeOopPtr::InstanceBot)->higher_equal(tn_t))
with
tinst->klass()->is_subtype_of(tn_t->klass())

And I found that I missed G1 barrier CallLeaf calls case
in process_call_arguments() (as result referenced allocation
was marked as global escape instead of thread local).

Thanks,
Vladimir

Vladimir Kozlov wrote:
> Thanks, Tom
> 
> Tom Rodriguez wrote:
>> I see what appear to be 4 copies of the Opcode() == Op_ClearArray 
>> logic.  Any chance of making one copy to be shared?
>>
> 
> Yes, I will move it into separate method and I will add ClearArray
> to NodeClasses to use is_ClearArray() instead of Opcode():
> 
> //----------------------------get_inst_memory---------------------------------- 
> 
> // Return allocation input memory edge if it is different instance
> // or itself if it is the one we are looking for.
> Node* ClearArrayNode::get_inst_memory(uint instance_id, PhaseTransform* 
> phase) {
>   intptr_t offset;
>   AllocateNode* alloc = AllocateNode::Ideal_allocation(this->in(3), 
> phase, offset);
>   // Can not bypass initialization of the instance
>   // we are looking for or when something is wrong.
>   if (alloc == NULL || alloc->_idx == instance_id)
>     return this;
>   // Otherwise skip it.
>   InitializeNode* init = alloc->initialization();
>   if (init != NULL)
>     return init->in(TypeFunc::Memory);
>   else
>     return alloc->in(TypeFunc::Memory);
> }
> 
>> escape.cpp:
>>
>> for the "// push allocation's users on appropriate worklist" code, 
>> shouldn't the if/then/else chain be terminated by an else 
>> ShouldNotReachHere or are there cases which don't have to be handled?  
>> If you put that in then you want to remove the ifdef ASSERT around the 
>> use->is_MergeMem case.
>>
> 
> There are cases which don't have to be handled (CmpP, for example).
> But you are right, I will add final "else" case and add asserts
> to check all known cases which don't need to be processed
> to make sure that interesting cases are not skipped.
> 
>> at line 1158, shouldn't the comment just be:
>>
>> // we don't need to do anything, but the users must be pushed
> 
> Done.
> 
>>
>> Otherwise I think this looks ok.
> 
> I will run tests over night with these changes and will send updated 
> webrev.
> 
> Thanks,
> Vladimir
> 
>>
>> tom
>>
>> On Nov 18, 2009, at 9:58 AM, Vladimir Kozlov wrote:
>>
>>> http://cr.openjdk.java.net/~kvn/6895383/webrev.04
>>>
>>> Fixed 6895383: JCK test throws NPE for method compiled with Escape 
>>> Analysis
>>>
>>> Problem:
>>> EA misses checks for MemBar nodes when looking for following
>>> MergeMem nodes. As result it did not add memory slice for
>>> the volatile store (followed by several MemBar nodes)
>>> in MergeMem of uncommon trap.
>>> So when the method deoptimized and eliminated object reallocated
>>> the NULL is stored to the corresponding field.
>>>
>>> Solution:
>>> Collect all MergeMem nodes at the beginning of EA when it walks
>>> through Ideal graph instead of searching MergeMem nodes by going
>>> down trough memory during memory splitting for non escaping objects.
>>> Also check for general MemBar nodes instead of only Initialize nodes
>>> during memory splitting.
>>>
>>> I also did next optimizations/fixes:
>>>
>>> 1. Check for SafePointScalarObject nodes to avoid printing empty lines
>>>   in PrintOptoAssembly output since they don't have corresponding 
>>> mach nodes.
>>>
>>> 2. Eliminate volatile MemBars nodes for stores into a scalar replaced 
>>> objects.
>>>   I noticed that we still generate membars even if the object is 
>>> eliminated.
>>>   For that I added the Precedent edge to corresponding Store node.
>>>
>>> 3. The check for ClearArray node is missing when searching for
>>>   a better memory edge during EA, memory optimization and an object
>>>   scalar replacement. We can't bypass it since it is the part of object
>>>   initialization (zeroing). I will try to factor it into separate 
>>> function
>>>   before push.
>>>
>>> 4. Add missing checks for string intrinsic nodes to adjust the escape 
>>> state
>>>   (non scalar replaceable) of char[] arrays they use.
>>>
>>> 5. Move code for searching objects which are not scalar replaceable 
>>> to separate
>>>   method verify_escape_state() since the code became too large. Add a 
>>> simple
>>>   control flow check to find initializing store for oop fields.
>>>
>>> Reviewed by:
>>>
>>> Fix verified (y/n): y, test
>>>
>>> Other testing:
>>> JPRT, CTW (32 and 64bit)
>>>
>>>
>>