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

Vladimir Kozlov Vladimir.Kozlov at Sun.COM
Wed Dec 9 09:57:03 PST 2009

Thanks, Tom


Tom Rodriguez wrote:
> Looks good.
> tom
> On Dec 4, 2009, at 5:50 PM, Vladimir Kozlov wrote:
>> 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:
>>>>> 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)

More information about the hotspot-compiler-dev mailing list