[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-compiler-dev
Subject:    Re: RFR: 8314997: Missing optimization opportunities due to missing try_clean_mem_phi() calls [v2]
From:       Vladimir Kozlov <kvn () openjdk ! org>
Date:       2023-08-30 16:37:13
Message-ID: sjKl72eJiy5jhxSXuBjMS4-d5u1L2RD1zbLx6bMhuaI=.81531a45-0223-433d-be13-d00d06d2f62b () github ! com
[Download RAW message or body]

On Wed, 30 Aug 2023 06:38:50 GMT, Christian Hagedorn <chagedorn@openjdk.org> wrote:

> > While working on a Valhalla bug, I've noticed that we sometimes miss \
> > `RegionNode::try_clean_mem_phi()` calls to remove a useless diamond 
> > If
> > True    False
> > Region
> > 
> > with only a single memory phi. This blocks further optimizations like converting \
> > a loop into a counted one. The code in Valhalla looks slightly different but the \
> > problem is also reproducible in mainline. 
> > **Problem**
> > 
> > In the test case, a region is transformed in IGVN such that it merges a diamond \
> > without any dependencies on both paths. The region has two phis. One of them is a \
> > memory phi which could be transformed by `RegionNode::try_clean_mem_phi()`. But \
> > when processing the region with its two phis in IGVN, we do not optimize the \
> > memory phi away because `has_unique_phi()` is false and we bail out: \
> > https://github.com/openjdk/jdk/blob/725ec0ce1b463b21cd4c5287cf4ccbee53ec7349/src/hotspot/share/opto/cfgnode.cpp#L450-L471
> >  
> > Later in IGVN, the second phi dies and we only have the single memory phi left. \
> > But the region will not be added to the IGVN worklist again to re-apply \
> > `try_clean_mem_phi()`. We therefore miss the removal of the diamond and we fail \
> > to apply further optimizations. In the test case, we fail to convert the loop \
> > into a counted loop. 
> > **Proposed Fix**
> > 
> > The fix I propose is to try to apply `try_clean_mem_phi()` whenever a region is \
> > merging a diamond with the assumption that the transformation of a memory phi \
> > does not hurt when being applied without being able to remove the region with the \
> > diamond (because there are other phis left that cannot be removed). Another \
> > option would be to re-add the region to the IGVN worklist when the second last \
> > phi dies. But the first approach seems simpler and less invasive. 
> > I've also applied some clean-ups and added an IR test.
> > 
> > Thanks,
> > Christian
> 
> Christian Hagedorn has updated the pull request incrementally with one additional \
> commit since the last revision: 
> Vladimir's review

Good.

-------------

Marked as reviewed by kvn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15445#pullrequestreview-1603101029


[prev in list] [next in list] [prev in thread] [next in thread]