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

[Vladimir Kozlov]

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
>