RFR: 8314997: Missing optimization opportunities due to missing try_clean_mem_phi() calls [v2]

[Vladimir Kozlov]

On Wed, 30 Aug 2023 06:38:50 GMT, Christian Hagedorn <chagedorn at 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