[8u-dev] Request for approval: 8164954: split_if creates empty phi and region nodes

[Kevin Walls]

Thanks Rob, done!



On 13/10/2017 23:38, Rob McKenna wrote:
> Approved. Please add an appropriate noreg label.
>
>      -Rob
>
> On 13/10/17 23:09, Kevin Walls wrote:
>> Hi,
>> I'd like to get backport approval for 8u-dev for:
>> bug: https://bugs.openjdk.java.net/browse/JDK-8164954
>>
>> 9 changeset: http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/38f38c10a11d
>>
>> 9 Review thread: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-March/025773.html
>>
>> Review of this 8u backport: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-October/027253.html
>>
>>
>> It doesn't quite hg import cleanly as the surrounding code is a little different, but the code change itself is unchanged.
>>
>> The 8u change has been through jprt testing and also tested with the testsuite of a Java-based product which was seen
>> hitting the same assert as in this bug.  hg diff of the proposed 8u change is below.
>>
>> Thanks!
>> Kevin
>>
>>
>>
>> -------- Forwarded Message --------
>> Subject: 	Re: [8u] RFF(S): 8164954: split_if creates empty phi and region
>> nodes
>> Date: 	Fri, 13 Oct 2017 10:57:46 -0700
>> From: 	Vladimir Kozlov <vladimir.kozlov at oracle.com>
>> To: 	Kevin Walls <kevin.walls at oracle.com>,
>> hotspot-compiler-dev at openjdk.java.net
>>
>>
>>
>> 8u change looks good.
>>
>> Thanks,
>> Vladimir
>>
>> On 10/13/17 2:25 AM, Kevin Walls wrote:
>>> Hi,
>>>
>>> I'd like to get a review of a backport to 8u.
>>>
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8164954
>>>
>>> 9 changeset: http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/38f38c10a11d
>>>
>>> Review thread: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-March/025773.html
>>>
>>>
>>> It doesn't hg import cleanly as the surrounding code is a little
>>> different: this change adds a condition in split_if() which may make that
>>> method return earlier, but 8u does not have the block after the change,
>>> beginning "if (nb_predicate_proj > 1) {", that comes in with 8078426.
>>>
>>> The 8u change has been through jprt testing and also tested with the
>>> testsuite of a Java-based product which was seen hitting the same assert
>>> as in this bug.  hg diff of the proposed 8u change is below, I think
>>> that's enough but can offer a webrev if anybody needs one.
>>>
>>> Thanks!
>>> Kevin
>>>
>>>
>>> bash-4.2$ hg diff src/share/vm/opto/ifnode.cpp
>>> diff -r c89173159237 src/share/vm/opto/ifnode.cpp
>>> --- a/src/share/vm/opto/ifnode.cpp      Thu Sep 07 10:15:21 2017 -0400
>>> +++ b/src/share/vm/opto/ifnode.cpp      Fri Oct 13 02:03:00 2017 -0700
>>> @@ -234,6 +234,13 @@
>>>         predicate_proj = proj;
>>>       }
>>>     }
>>> +
>>> +  // If all the defs of the phi are the same constant, we already have the desired end state.
>>> +  // Skip the split that would create empty phi and region nodes.
>>> +  if((r->req() - req_c) == 1) {
>>> +    return NULL;
>>> +  }
>>> +
>>>     Node* predicate_c = NULL;
>>>     Node* predicate_x = NULL;
>>>     bool counted_loop = r->is_CountedLoop();
>>>
>>>