RFR: JDK-8275854: C2: assert(stride_con != 0) failed: missed some peephole opt [v3]

[Roland Westrelin]

On Tue, 26 Oct 2021 01:42:41 GMT, 王超 <duke at openjdk.java.net> wrote:

>> `If subsume` optimization will eliminate `LongCountedLoopEndNode` node by mistake, which will lead to `PhaseIdealLoop` optimization crash.
>> 
>> For example, the test of node 538 and node 553 will become the same after the first `PhaseIdealLoop` optimization. Node 555 is the back edge to the loop, and node 553 will be replaced by a `LongCountedLoopEndNode` node.
>> <img width="592" alt="image" src="https://user-images.githubusercontent.com/25214855/138656330-9f825c85-0468-40a5-9eb9-b576130648db.png">
>> 
>> 
>>  In the next `PhaseIdealLoop` optimization, node 538 find node 553 is redundant, and will subsume node 553. Then the `PhaseIdealLoop` optimization will crash, because there is no loop end node.
>> <img width="603" alt="image" src="https://user-images.githubusercontent.com/25214855/138656951-53685723-92c2-42d6-91ae-445107dcb01d.png">
>> 
>> There are two way to fix the crash, the first is like the way in this pr, just exit `IFNode subsume` optimization when it's a `LongCountedLoopEndNode` node. The second possible fix is that exchange the dominating `IF` node with the `LongCountedLoopEndNode` node:
>> 
>> diff --git a/src/hotspot/share/opto/ifnode.cpp b/src/hotspot/share/opto/ifnode.cpp
>> index 38b40a6..31ff172 100644
>> --- a/src/hotspot/share/opto/ifnode.cpp
>> +++ b/src/hotspot/share/opto/ifnode.cpp
>> @@ -1674,6 +1674,21 @@ Node* IfNode::simple_subsuming(PhaseIterGVN* igvn) {
>>      }
>>    }
>>  
>> +  if (is_LongCountedLoopEnd()) {
>> +    set_req(0, dom->in(0));
>> +    set_req(1, dom->in(1));
>> +    dom->set_req(0, pre);
>> +    dom->set_req(1, igvn->intcon(is_always_true ? 1 : 0));
>> +    Node* proj0 = raw_out(0);
>> +    Node* proj1 = raw_out(1);
>> +    Node* dom_proj0 = dom->raw_out(0);
>> +    Node* dom_proj1 = dom->raw_out(1);
>> +    dom_proj0->set_req(0, this);
>> +    dom_proj1->set_req(0, this);
>> +    proj0->set_req(0, dom);
>> +    proj1->set_req(0, dom);
>> +  }
>> +
>>    if (bol->outcnt() == 0) {
>>      igvn->remove_dead_node(bol);    // Kill the BoolNode.
>>    }
>> diff --git a/src/hotspot/share/opto/loopnode.cpp b/src/hotspot/share/opto/loopnode.cpp
>> index 6f7e34d..7955722 100644
>> --- a/src/hotspot/share/opto/loopnode.cpp
>> +++ b/src/hotspot/share/opto/loopnode.cpp
>> @@ -802,7 +802,7 @@ bool PhaseIdealLoop::transform_long_counted_loop(IdealLoopTree* loop, Node_List
>>    Node* back_control = head->in(LoopNode::LoopBackControl);
>>  
>>    // data nodes on back branch not supported
>> -  if (back_control->outcnt() > 1) {
>> +  if (back_control->outcnt() > 1 || back_control->Opcode() != Op_IfTrue) {
>>      return false;
>>    }
>
> 王超 has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Adjust the code style

The optimization is valid AFAIU, so I don't think blocking it is the right fix. What about something like this:

diff --git a/src/hotspot/share/opto/ifnode.cpp b/src/hotspot/share/opto/ifnode.cpp
index 38b40a68b1f..b28a382ebe7 100644
--- a/src/hotspot/share/opto/ifnode.cpp
+++ b/src/hotspot/share/opto/ifnode.cpp
@@ -1721,6 +1721,14 @@ Node* IfProjNode::Identity(PhaseGVN* phase) {
        // will cause this node to be reprocessed once the dead branch is killed.
        in(0)->outcnt() == 1))) {
     // IfNode control
+    if (in(0)->is_BaseCountedLoopEnd()) {
+      Node* head = unique_ctrl_out();
+      if (head != NULL && head->is_BaseCountedLoop() && head->in(LoopNode::LoopBackControl) == this) {
+        Node* new_head = new LoopNode(head->in(LoopNode::EntryControl), this);
+        phase->is_IterGVN()->register_new_node_with_optimizer(new_head);
+        phase->is_IterGVN()->replace_node(head, new_head);
+      }
+    }
     return in(0)->in(0);
   }
   // no progress

That prevents the crash and gives the loop another chance to be optimized.

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

PR: https://git.openjdk.java.net/jdk/pull/6099