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

[Vladimir Kozlov]

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