RFR(M): 8130847: Cloned object's fields observed as null after C2 escape analysis

[Vladimir Kozlov]

Looks good to me.

Thanks,
Vladimir

On 7/30/15 11:29 AM, Roland Westrelin wrote:
> Updated webrev with Vladimir’s comments:
>
> http://cr.openjdk.java.net/~roland/8130847/webrev.01/
>
> Roland.
>
>> On Jul 30, 2015, at 2:48 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>
>> On 7/29/15 6:57 AM, Roland Westrelin wrote:
>>>> The next change puzzles me:
>>>>
>>>> -         if (!call->may_modify(tinst, phase)) {
>>>> +         if (call->may_modify(tinst, phase)) {
>>>> -           mem = call->in(TypeFunc::Memory);
>>>> +           assert(call->is_ArrayCopy(), "ArrayCopy is the only call node that doesn't make allocation escape");
>>>>
>>>> Why only ArrayCopy? I think it is most of calls. What set of tests you ran?
>>>>
>>>> Methods naming is confusing. membar_for_arraycopy() does not check for membar but for calls which can modify. handle_arraycopy() could be make_arraycopy_load().
>>>
>>> What about:
>>>
>>> static bool may_modify(const TypeOopPtr *t_oop, MemBarNode* mb, PhaseTransform *phase);
>>>
>>> instead of membar_for_arraycopy()
>>>
>>> So ArrayCopyNode would have:
>>>
>>> virtual bool may_modify(const TypeOopPtr *t_oop, PhaseTransform *phase);
>>>
>>> and
>>>
>>> static bool may_modify(const TypeOopPtr *t_oop, MemBarNode* mb, PhaseTransform *phase);
>>>
>>> that do the same thing except the static method also looks for a graph pattern starting from a MemBar.
>>
>> Yes, it is better.
>>
>> Thanks,
>> Vladimir
>>
>>>
>>> Roland.
>>>
>>>>
>>>> Add explicit check:
>>>> && strcmp(_name, "unsafe_arraycopy") != 0)
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 7/28/15 7:05 AM, Roland Westrelin wrote:
>>>>> http://cr.openjdk.java.net/~roland/8130847/webrev.00/
>>>>>
>>>>> When an allocation which is the destination of an ArrayCopyNode is eliminated, field’s values recorded at a safepoint (to reallocate the object) do not take the ArrayCopyNode into account at all and the effect or the ArrayCopyNode is lost on a deoptimization. This fix records values from the source of the ArrayCopyNode, emitting new loads if necessary.
>>>>>
>>>>> I also use the opportunity to pin the loads generated in LoadNode::can_see_arraycopy_value() because they depend on all checks that validate the array copy and not only on the check that immediately dominates.
>>>>>
>>>>> Roland.
>