RFR: 8144484: assert(no_dead_loop) failed: dead loop detected

[Rahul Raghavan]

Hi,

Please review following fix patch for JDK-8144484.

<webrev.00> -  http://cr.openjdk.java.net/~rraghavan/8144484/webrev.00/

Please check the details in <jbs> https://bugs.openjdk.java.net/browse/JDK-8144484


1. Found the root cause of the issue as strong dead loop checks not done in PhiNode::Ideal()
  before splitting Phi through memory merges,
  before new MergeMemNodes creation, which is not dead_loop_safe.


2. Above webrev.00 fix proposal in PhiNode::Ideal() [src/share/vm/opto/cfgnode.cpp]
Related code extracts for reference -

- - - - - - - - - - - - - - - - - - - - - - - - -
// Return a node which is more "ideal" than the current node. Must preserve
// the CFG, but we can still strip out dead paths.
Node *PhiNode::Ideal(PhaseGVN *phase, bool can_reshape) {
  ...........
  ...........
  // Split phis through memory merges, so that the memory merges will go away.
  // Piggy-back this transformation on the search for a unique input....
  // It will be as if the merged memory is the unique value of the phi.
  // (Do not attempt this optimization unless parsing is complete.
  // It would make the parser's memory-merge logic sick.)
  // (MergeMemNode is not dead_loop_safe - need to check for dead loop.)
  if (progress == NULL && can_reshape && type() == Type::MEMORY) {
    ...........
    ...........
       // We know that at least one MergeMem->base_memory() == this
        // (saw_self == true). If all other inputs also references this phi
        // (directly or through data nodes) - it is dead loop.
         bool saw_safe_input = false;
         for (uint j = 1; j < req(); ++j) {
           Node *n = in(j);
           if (n->is_MergeMem() && n->as_MergeMem()->base_memory() == this) {
             continue; // skip known cases
           }
+        // TOP inputs should not be counted as safe inputs because if the
+        // Phi references itself through all other inputs then splitting the
+        // Phi through memory merges would create dead loop at later stage..
+        if (n == top) {
+        continue;
+        }
           if (!is_unsafe_data_reference(n)) {
             saw_safe_input = true; // found safe input
             break;
          }
        }
        if (!saw_safe_input)
          return top; // all inputs reference back to this phi - dead loop

        // Phi(...MergeMem(m0, m1:AT1, m2:AT2)...) into
        // MergeMem(Phi(...m0...), Phi:AT1(...m1...), Phi:AT2(...m2...))
   ...................
- - - - - - - - - - - - - - - - - - - - - - - - -


3. For the failing test case, following is the related input graph nodes dump in PhiNode::Ideal() before checking for possible dead loop
    (before confirming saw_safe_input)

      2808 Region === 2808 1 2980 2804 [[ 2808 3002 2987 2985 ]] ..........
          2980 IfFalse === 2978 [[ 2808 ]] ..........
          2804 IfTrue === 2803 [[ 2808 ]] ..........
      2985 Phi === 2808 1 6044 3011 [[ 3020 3003 3026 3011 2860 2785 2769 2821 2833 2943 2903 ]] ..........
          6044 MergeMem === _ 1 3011 1 1 1 6045 [[ 6050 6048 2985 ]] ..........
          3011 MergeMem === _ 1 2985 1 1 1 2987 [[ 2987 2985 6045 6044 ]] ..........

Wrongly considering TOP inputs as safe inputs results in dead loop at later stage.
Without the fix, related graph nodes dumped during failing case -
   3011 MergeMem === _ 1 6058 1 1 1 2987 [[ 2987 6058 6045 6044 ]] ...
   6058 MergeMem === _ 1 3011 1 1 1 6059 [[ 2903 2943 2833 2821 2769 2785 2860 3011 3026 3003 3020 ]] ...
The dead loop causing the crash -- [6058] > [3011] > [6058] > [3011]


4. So proposed webrev.00 fix in PhiNode::Ideal() is to ignore the TOP inputs before checking the is_unsafe_data_reference(n),
  so that saw_safe_input is set to the correct value.
TOP inputs should be ignored else if there is an actual dead loop
  .i.e. if the Phi references to itself through all other inputs
  then splitting the Phi through memory merges would create dead loop at later stage
  because Phi with TOP inputs will go away exposing the dead loop.


Confirmed the fix with 8144484 test and JPRT (-testset hotspot). RBT Pre-integration testing in progress.


Thanks,
Rahul