JDK-8257197 : Add additional verification code to PhaseCCP
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 16,17
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2020-11-27
  • Updated: 2023-08-03
  • Resolved: 2022-12-09
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 21
21 b02Fixed
Related Reports
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Description
We should catch problems like JDK-8257166 during CCP by adding some verification code:

@@ -1717,11 +1717,13 @@ void PhaseCCP::analyze() {
   // Push root onto worklist
   Unique_Node_List worklist;
   worklist.push(C->root());
+  DEBUG_ONLY(Unique_Node_List worklist_verify;)
 
   // Pull from worklist; compute new value; push changes out.
   // This loop is the meat of CCP.
   while( worklist.size() ) {
     Node *n = worklist.pop();
+    DEBUG_ONLY(worklist_verify.push(n);)
     const Type *t = n->Value(this);
     if (t != type(n)) {
       assert(ccp_type_widens(t, type(n)), "ccp type must widen");
@@ -1813,6 +1815,21 @@ void PhaseCCP::analyze() {
       }
     }
   }
+#ifdef ASSERT
+  while (worklist_verify.size()) {
+    Node* n = worklist_verify.pop();
+    const Type* told = type(n);
+    const Type* tnew = n->Value(this);
+    if (told != tnew) {
+      told->dump_on(tty);
+      tty->print_cr("");
+      tnew->dump_on(tty);
+      tty->print_cr("");
+      n->dump(1);
+      fatal("missed optimization opportunity");
+    }
+  }
+#endif
 }
Comments
Changeset: 11aece21 Author: Emanuel Peter <epeter@openjdk.org> Date: 2022-12-09 07:11:57 +0000 URL: https://git.openjdk.org/jdk/commit/11aece21f4eb5b18af357b265bc27b80bcdbfbcb
09-12-2022

A pull request was submitted for review. URL: https://git.openjdk.org/jdk/pull/11529 Date: 2022-12-06 08:02:12 +0000
06-12-2022

A few observations: The main reason that optimizations that could have happened did not happen: some (recursive) input node had a modification but did not notify down the outputs enough. The problem: the further the optimization looks recursively at the inputs, the more we would have to notify down modifications. Currently, we do 1 hop by default, in some cases 2 hops, and in special occasions 3 hops. Further: during IGVN and CCP we have different notification implementations that do not agree. PhaseCCP::push_child_nodes_to_worklist PhaseIterGVN::add_users_to_worklist The differences may be in part explained by the difference in optimizatzions of IGVN and CCP, but who really checks that it is correct? It is very easy to forget notification all together, or in one of the two locations. So that I can make progress, I will exclude LoadNodes from the verification for now.
30-09-2022

We can do a similar thing in IGVN. I have noticed that for example AddINode checks input of input to do constant folding. But when an input of an input is changed during IGVN, only that changed node's outputs get notified and put on the igvn worklist, and not the AddINode which is an output of an output. We do that "grandchildren" notification in some cases though - just not in all.
22-09-2022

Also seeing this which is not as straight forward to fix: char char:w 2051 Phi === 2045 2036 275 [[ 2118 2152 2087 2118 2110 3474 2170 2132 2135 2141 2152 ]] #int:-257..65535 !jvms: DataInputStream::readUTF @ bci:-1 (line 595) 2166 Proj === 2165 [[ 2183 2169 2170 ]] #0 !jvms: DataInputStream::readUTF @ bci:70 (line 607) 2170 CastII === 2166 2051 [[ 2178 2183 2183 ]] #char !jvms: DataInputStream::readUTF @ bci:70 (line 607)
30-11-2020

Adding this verification code triggers failures right away. One issue: int bool 73 Conv2B === _ 186 [[ 77 ]] !jvms: Assert::assertEquals @ bci:27 (line 115) 59 Conv2B === _ 57 [[ 77 ]] !jvms: Assert::assertEquals @ bci:18 (line 115) 77 XorI === _ 59 73 [[ 78 ]] !orig=74 !jvms: Assert::assertEquals @ bci:27 (line 115) XorINode::add_ring should be fixed like this: // Complementing a boolean? - if( r0 == TypeInt::BOOL && ( r1 == TypeInt::ONE - || r1 == TypeInt::BOOL)) + if ((r0 == TypeInt::BOOL && r1 == TypeInt::ONE) || + (r0 == TypeInt::ONE && r1 == TypeInt::BOOL)) { return TypeInt::BOOL; + }
30-11-2020