Re: 回复：[Patch] 8210853: C2 doesn't skip post barrier for new allocated objects

[Vladimir Kozlov]

Sorry, I mean

"should exist Phi *associated* with allocation"

On 10/3/18 3:48 PM, Vladimir Kozlov wrote:
> Hi Kevin,
> 
> You correctly pointed in your analysis that if base of address is pointing to allocation there 
> should not be other control path to this store. But it also means that before there should exist Phi 
> not associated with allocation when Region node was added. If Phi node was removed at some time 
> Region node should be removed too (or not add it in first place). Please, look on that.
> 
> I share Tobias's concern about skipping Region node in Parse phase when IR graph is still constructed.
> 
> Thanks,
> Vladimir
> 
> On 10/3/18 8:08 AM, Kuai Wei wrote:
>> Hi Tobias,
>>
>>   I made the change to check with RegionNode::is_copy, could you check the new patch?
>>
>> diff --git a/src/hotspot/share/opto/graphKit.cpp b/src/hotspot/share/opto/graphKit.cpp
>> index 068141f..884a76c 100644
>> --- a/src/hotspot/share/opto/graphKit.cpp
>> +++ b/src/hotspot/share/opto/graphKit.cpp
>> @@ -2126,7 +2126,15 @@ void GraphKit::uncommon_trap(int trap_request,
>>   // We use this to determine if an object is so "fresh" that
>>   // it does not require card marks.
>>   Node* GraphKit::just_allocated_object(Node* current_control) {
>> -  if (C->recent_alloc_ctl() == current_control)
>> +  Node * ctrl = current_control;
>> +  // Object::<init> is invoked after allocation, most of invoke nodes
>> +  // will be reduced, but a region node is kept in parse time, we check
>> +  // the pattern and skip the region node
>> +  if (ctrl != NULL && ctrl->is_Region() && ctrl->as_Region()->is_copy()) {
>> +    assert(ctrl->req() == 2, "copy region has only 2 inputs");
>> +    ctrl = ctrl->as_Region()->is_copy();
>> +  }
>> +  if (C->recent_alloc_ctl() == ctrl)
>>       return C->recent_alloc_obj();
>>     return NULL;
>>   }
>> Thanks,
>> Kevin
>>
>>     ------------------------------------------------------------------
>>     From:蒯微(麦庶) <kuaiwei.kw at alibaba-inc.com>
>>     Send Time:2018年9月25日(星期二) 21:50
>>     To:Tobias Hartmann <tobias.hartmann at oracle.com>; hotspot compiler
>>     <hotspot-compiler-dev at openjdk.java.net>
>>     Cc:李三红(三红) <sanhong.lsh at alibaba-inc.com>
>>     Subject:回复：回复：[Patch] 8210853: C2 doesn't skip post barrier for new allocated objects
>>
>>     Hi Tobias,
>>
>>        Thanks for your comments. I will check RegionNode::is_copy to see if it can be used to detect
>>     unnecessary region node. I will send new review after testing.
>>
>>     Best Regards,
>>     Kevin
>>
>>     ------------------------------------------------------------------
>>     发件人：Tobias Hartmann <tobias.hartmann at oracle.com>
>>     发送时间：2018年9月24日(星期一) 21:34
>>     收件人：蒯微(麦庶) <kuaiwei.kw at alibaba-inc.com>; hotspot compiler
>>     <hotspot-compiler-dev at openjdk.java.net>
>>     抄　送：李三红(三红) <sanhong.lsh at alibaba-inc.com>
>>     主　题：Re: 回复：[Patch] 8210853: C2 doesn't skip post barrier for new allocated objects
>>
>>     Hi Kevin,
>>
>>     On 24.09.2018 08:06, Kuai Wei wrote:
>>      
>> >   Thanks for your suggestion. I think your point is the region node may have new path in later parse 
>>
>>      > phase, so we can not make sure the region node will be optimized.
>>
>>     
>> Yes, my point is that a new path to the region might be added after your optimization and that path
>>     might contain stores to the newly allocated object.
>>
>>      >   It's a good question and I checked it. Now I think it may not cause trouble. In post barrier
>>      > reduce, the oop store use allocation node as base pointer. The data graph guarantee control of
>>      
>> > allocation node should dominate control of store. If allocation node is in pred of region node and
>>      
>> > there's a new path into region, the graph is bad because we can reach store without allocation.
>>
>>     Yes but the new path might be a backedge from a loop that is dominated by the allocation.
>>
>>      
>> > If allocation node is in a domination ancestor, the graph shape is a little complicated, so we can not 
>>
>>      > reach control of allocation by skipping one region.
>>
>>     
>> Right, that's basically the implicit assumption of your patch. I'm not sure if it always holds. But
>>     I think you should at least use RegionNode::is_copy().
>>
>>     Let's see what other reviewers think.
>>
>>      
>> >   The better solution is we can know the region node is created in exit_map and we will not change
>>      > it in later. Is there any way to know it in compile time?
>>
>>     
>> The region node is created in Parse::build_exits(). I don't think there is a way to keep track of this. 
>>
>>
>>     Thanks,
>>     Tobias
>>