8256823: C2 compilation fails with "assert(isShiftCount(imm8 >> 1)) failed: illegal shift count"

Reviewed-by: vlivanov, kvn, chagedorn

src/hotspot/cpu/x86/assembler_x86.cpp

@@ -4905,7 +4905,7 @@ void Assembler::ret(int imm16) {
 }
 
 void Assembler::roll(Register dst, int imm8) {
-  assert(isShiftCount(imm8 >> 1), "illegal shift count");
+  assert(isShiftCount(imm8), "illegal shift count");
   int encode = prefix_and_encode(dst->encoding());
   if (imm8 == 1) {
     emit_int16((unsigned char)0xD1, (0xC0 | encode));
@@ -4920,7 +4920,7 @@ void Assembler::roll(Register dst) {
 }
 
 void Assembler::rorl(Register dst, int imm8) {
-  assert(isShiftCount(imm8 >> 1), "illegal shift count");
+  assert(isShiftCount(imm8), "illegal shift count");
   int encode = prefix_and_encode(dst->encoding());
   if (imm8 == 1) {
     emit_int16((unsigned char)0xD1, (0xC8 | encode));

src/hotspot/share/opto/addnode.cpp

@@ -767,22 +767,22 @@ Node* OrINode::Ideal(PhaseGVN* phase, bool can_reshape) {
   int ropcode = in(2)->Opcode();
   if (Matcher::match_rule_supported(Op_RotateLeft) &&
       lopcode == Op_LShiftI && ropcode == Op_URShiftI && in(1)->in(1) == in(2)->in(1)) {
-     Node* lshift = in(1)->in(2);
+    Node* lshift = in(1)->in(2);
-     Node* rshift = in(2)->in(2);
+    Node* rshift = in(2)->in(2);
-     Node* shift = rotate_shift(phase, lshift, rshift, 0x1F);
+    Node* shift = rotate_shift(phase, lshift, rshift, 0x1F);
-     if (shift != NULL) {
+    if (shift != NULL) {
-       return new RotateLeftNode(in(1)->in(1), shift, TypeInt::INT);
+      return new RotateLeftNode(in(1)->in(1), shift, TypeInt::INT);
-     }
+    }
-     return NULL;
+    return NULL;
   }
   if (Matcher::match_rule_supported(Op_RotateRight) &&
       lopcode == Op_URShiftI && ropcode == Op_LShiftI && in(1)->in(1) == in(2)->in(1)) {
-     Node *rshift = in(1)->in(2);
+    Node* rshift = in(1)->in(2);
-     Node *lshift = in(2)->in(2);
+    Node* lshift = in(2)->in(2);
-     Node* shift = rotate_shift(phase, rshift, lshift, 0x1F);
+    Node* shift = rotate_shift(phase, rshift, lshift, 0x1F);
-     if (shift != NULL) {
+    if (shift != NULL) {
-       return new RotateRightNode(in(1)->in(1), shift, TypeInt::INT);
+      return new RotateRightNode(in(1)->in(1), shift, TypeInt::INT);
-     }
+    }
   }
   return NULL;
 }

src/hotspot/share/opto/mulnode.cpp

@@ -637,35 +637,44 @@ Node *AndLNode::Ideal(PhaseGVN *phase, bool can_reshape) {
 
 //=============================================================================
 
-static int getShiftCon(PhaseGVN *phase, Node *shiftNode, int retVal) {
+static bool const_shift_count(PhaseGVN* phase, Node* shiftNode, int* count) {
-  const Type *t = phase->type(shiftNode->in(2));
+  const TypeInt* tcount = phase->type(shiftNode->in(2))->isa_int();
-  if (t == Type::TOP) return retVal;       // Right input is dead.
+  if (tcount != NULL && tcount->is_con()) {
-  const TypeInt *t2 = t->isa_int();
+    *count = tcount->get_con();
-  if (!t2 || !t2->is_con()) return retVal; // Right input is a constant.
+    return true;
-
+  }
-  return t2->get_con();
+  return false;
 }
 
-static int maskShiftAmount(PhaseGVN *phase, Node *shiftNode, int nBits) {
+static int maskShiftAmount(PhaseGVN* phase, Node* shiftNode, int nBits) {
-  int       shift = getShiftCon(phase, shiftNode, 0);
+  int count = 0;
-  int maskedShift = shift & (nBits - 1);
+  if (const_shift_count(phase, shiftNode, &count)) {
-
+    int maskedShift = count & (nBits - 1);
-  if (maskedShift == 0) return 0;         // Let Identity() handle 0 shift count.
+    if (maskedShift == 0) {
-
+      // Let Identity() handle 0 shift count.
-  if (shift != maskedShift) {
+      return 0;
-    shiftNode->set_req(2, phase->intcon(maskedShift)); // Replace shift count with masked value.
-    PhaseIterGVN* igvn = phase->is_IterGVN();
-    if (igvn) {
-      igvn->rehash_node_delayed(shiftNode);
     }
-  }
 
-  return maskedShift;
+    if (count != maskedShift) {
+      shiftNode->set_req(2, phase->intcon(maskedShift)); // Replace shift count with masked value.
+      PhaseIterGVN* igvn = phase->is_IterGVN();
+      if (igvn) {
+        igvn->rehash_node_delayed(shiftNode);
+      }
+    }
+    return maskedShift;
+  }
+  return 0;
 }
 
 //------------------------------Identity---------------------------------------
 Node* LShiftINode::Identity(PhaseGVN* phase) {
-  return ((getShiftCon(phase, this, -1) & (BitsPerJavaInteger - 1)) == 0) ? in(1) : this;
+  int count = 0;
+  if (const_shift_count(phase, this, &count) && (count & (BitsPerJavaInteger - 1)) == 0) {
+    // Shift by a multiple of 32 does nothing
+    return in(1);
+  }
+  return this;
 }
 
 //------------------------------Ideal------------------------------------------
@@ -773,7 +782,12 @@ const Type* LShiftINode::Value(PhaseGVN* phase) const {
 //=============================================================================
 //------------------------------Identity---------------------------------------
 Node* LShiftLNode::Identity(PhaseGVN* phase) {
-  return ((getShiftCon(phase, this, -1) & (BitsPerJavaLong - 1)) == 0) ? in(1) : this;
+  int count = 0;
+  if (const_shift_count(phase, this, &count) && (count & (BitsPerJavaLong - 1)) == 0) {
+    // Shift by a multiple of 64 does nothing
+    return in(1);
+  }
+  return this;
 }
 
 //------------------------------Ideal------------------------------------------
@@ -878,26 +892,30 @@ const Type* LShiftLNode::Value(PhaseGVN* phase) const {
 //=============================================================================
 //------------------------------Identity---------------------------------------
 Node* RShiftINode::Identity(PhaseGVN* phase) {
-  int shift = getShiftCon(phase, this, -1);
+  int count = 0;
-  if (shift == -1) return this;
+  if (const_shift_count(phase, this, &count)) {
-  if ((shift & (BitsPerJavaInteger - 1)) == 0) return in(1);
+    if ((count & (BitsPerJavaInteger - 1)) == 0) {
-
+      // Shift by a multiple of 32 does nothing
-  // Check for useless sign-masking
+      return in(1);
-  if (in(1)->Opcode() == Op_LShiftI &&
+    }
-      in(1)->req() == 3 &&
+    // Check for useless sign-masking
-      in(1)->in(2) == in(2)) {
+    if (in(1)->Opcode() == Op_LShiftI &&
-    shift &= BitsPerJavaInteger-1; // semantics of Java shifts
+        in(1)->req() == 3 &&
-    // Compute masks for which this shifting doesn't change
+        in(1)->in(2) == in(2)) {
-    int lo = (-1 << (BitsPerJavaInteger - ((uint)shift)-1)); // FFFF8000
+      count &= BitsPerJavaInteger-1; // semantics of Java shifts
-    int hi = ~lo;               // 00007FFF
+      // Compute masks for which this shifting doesn't change
-    const TypeInt *t11 = phase->type(in(1)->in(1))->isa_int();
+      int lo = (-1 << (BitsPerJavaInteger - ((uint)count)-1)); // FFFF8000
-    if (!t11) return this;
+      int hi = ~lo;               // 00007FFF
-    // Does actual value fit inside of mask?
+      const TypeInt* t11 = phase->type(in(1)->in(1))->isa_int();
-    if (lo <= t11->_lo && t11->_hi <= hi) {
+      if (t11 == NULL) {
-      return in(1)->in(1);      // Then shifting is a nop
+        return this;
+      }
+      // Does actual value fit inside of mask?
+      if (lo <= t11->_lo && t11->_hi <= hi) {
+        return in(1)->in(1);      // Then shifting is a nop
+      }
     }
   }
-
   return this;
 }
 
@@ -1082,8 +1100,11 @@ const Type* RShiftLNode::Value(PhaseGVN* phase) const {
 //=============================================================================
 //------------------------------Identity---------------------------------------
 Node* URShiftINode::Identity(PhaseGVN* phase) {
-  int shift = getShiftCon(phase, this, -1);
+  int count = 0;
-  if ((shift & (BitsPerJavaInteger - 1)) == 0) return in(1);
+  if (const_shift_count(phase, this, &count) && (count & (BitsPerJavaInteger - 1)) == 0) {
+    // Shift by a multiple of 32 does nothing
+    return in(1);
+  }
 
   // Check for "((x << LogBytesPerWord) + (wordSize-1)) >> LogBytesPerWord" which is just "x".
   // Happens during new-array length computation.
@@ -1266,7 +1287,12 @@ const Type* URShiftINode::Value(PhaseGVN* phase) const {
 //=============================================================================
 //------------------------------Identity---------------------------------------
 Node* URShiftLNode::Identity(PhaseGVN* phase) {
-  return ((getShiftCon(phase, this, -1) & (BitsPerJavaLong - 1)) == 0) ? in(1) : this;
+  int count = 0;
+  if (const_shift_count(phase, this, &count) && (count & (BitsPerJavaLong - 1)) == 0) {
+    // Shift by a multiple of 64 does nothing
+    return in(1);
+  }
+  return this;
 }
 
 //------------------------------Ideal------------------------------------------
@@ -1444,6 +1470,21 @@ uint MulAddS2INode::hash() const {
 
 //------------------------------Rotate Operations ------------------------------
 
+Node* RotateLeftNode::Identity(PhaseGVN* phase) {
+  const Type* t1 = phase->type(in(1));
+  if (t1 == Type::TOP) {
+    return this;
+  }
+  int count = 0;
+  assert(t1->isa_int() || t1->isa_long(), "Unexpected type");
+  int mask = (t1->isa_int() ? BitsPerJavaInteger : BitsPerJavaLong) - 1;
+  if (const_shift_count(phase, this, &count) && (count & mask) == 0) {
+    // Rotate by a multiple of 32/64 does nothing
+    return in(1);
+  }
+  return this;
+}
+
 const Type* RotateLeftNode::Value(PhaseGVN* phase) const {
   const Type* t1 = phase->type(in(1));
   const Type* t2 = phase->type(in(2));
@@ -1460,11 +1501,10 @@ const Type* RotateLeftNode::Value(PhaseGVN* phase) const {
     if (r1 == TypeInt::ZERO) {
       return TypeInt::ZERO;
     }
-    // Shift by zero does nothing
+    // Rotate by zero does nothing
     if (r2 == TypeInt::ZERO) {
       return r1;
     }
-
     if (r1->is_con() && r2->is_con()) {
       int shift = r2->get_con() & (BitsPerJavaInteger - 1); // semantics of Java shifts
       return TypeInt::make((r1->get_con() << shift) | (r1->get_con() >> (32 - shift)));
@@ -1479,11 +1519,10 @@ const Type* RotateLeftNode::Value(PhaseGVN* phase) const {
     if (r1 == TypeLong::ZERO) {
       return TypeLong::ZERO;
     }
-    // Shift by zero does nothing
+    // Rotate by zero does nothing
     if (r2 == TypeInt::ZERO) {
       return r1;
     }
-
     if (r1->is_con() && r2->is_con()) {
       int shift = r2->get_con() & (BitsPerJavaLong - 1); // semantics of Java shifts
       return TypeLong::make((r1->get_con() << shift) | (r1->get_con() >> (64 - shift)));
@@ -1508,6 +1547,21 @@ Node* RotateLeftNode::Ideal(PhaseGVN *phase, bool can_reshape) {
   return NULL;
 }
 
+Node* RotateRightNode::Identity(PhaseGVN* phase) {
+  const Type* t1 = phase->type(in(1));
+  if (t1 == Type::TOP) {
+    return this;
+  }
+  int count = 0;
+  assert(t1->isa_int() || t1->isa_long(), "Unexpected type");
+  int mask = (t1->isa_int() ? BitsPerJavaInteger : BitsPerJavaLong) - 1;
+  if (const_shift_count(phase, this, &count) && (count & mask) == 0) {
+    // Rotate by a multiple of 32/64 does nothing
+    return in(1);
+  }
+  return this;
+}
+
 const Type* RotateRightNode::Value(PhaseGVN* phase) const {
   const Type* t1 = phase->type(in(1));
   const Type* t2 = phase->type(in(2));
@@ -1524,7 +1578,7 @@ const Type* RotateRightNode::Value(PhaseGVN* phase) const {
     if (r1 == TypeInt::ZERO) {
       return TypeInt::ZERO;
     }
-    // Shift by zero does nothing
+    // Rotate by zero does nothing
     if (r2 == TypeInt::ZERO) {
       return r1;
     }
@@ -1533,7 +1587,6 @@ const Type* RotateRightNode::Value(PhaseGVN* phase) const {
       return TypeInt::make((r1->get_con() >> shift) | (r1->get_con() << (32 - shift)));
     }
     return TypeInt::INT;
-
   } else {
     assert(t1->isa_long(), "Type must be a long");
     const TypeLong* r1 = t1->is_long();
@@ -1542,7 +1595,7 @@ const Type* RotateRightNode::Value(PhaseGVN* phase) const {
     if (r1 == TypeLong::ZERO) {
       return TypeLong::ZERO;
     }
-    // Shift by zero does nothing
+    // Rotate by zero does nothing
     if (r2 == TypeInt::ZERO) {
       return r1;
     }

src/hotspot/share/opto/mulnode.hpp

@@ -220,6 +220,7 @@ class RotateLeftNode : public TypeNode {
     init_req(2, in2);
   }
   virtual int Opcode() const;
+  virtual Node* Identity(PhaseGVN* phase);
   virtual const Type* Value(PhaseGVN* phase) const;
   virtual Node* Ideal(PhaseGVN* phase, bool can_reshape);
 };
@@ -232,6 +233,7 @@ class RotateRightNode : public TypeNode {
     init_req(2, in2);
   }
   virtual int Opcode() const;
+  virtual Node* Identity(PhaseGVN* phase);
   virtual const Type* Value(PhaseGVN* phase) const;
 };
 

test/hotspot/jtreg/compiler/intrinsics/TestRotate.java

@@ -23,7 +23,7 @@
 
 /**
  * @test
- * @bug 8248830
+ * @bug 8248830 8256823
  * @summary Support for scalar rotates ([Integer/Long].rotate[Left/Right]).
  * @library /test/lib
  * @requires vm.compiler2.enabled
@@ -280,6 +280,62 @@ public class TestRotate {
       verify("Constant long rotateRight pattern = 129", res2 , ref_long_ror_shift_M129[index]);
     }
 
+    public static void test_rol_int_zero(int val) {
+        // Count is known to be zero only after loop opts
+        int count = 42;
+        for (int i = 0; i < 4; i++) {
+            if ((i % 2) == 0) {
+                count = 0;
+            }
+        }
+        int res = Integer.rotateLeft(val, count);
+        if (res != val) {
+            throw new RuntimeException("test_rol_int_zero failed: " + res + " != " + val);
+        }
+    }
+
+    public static void test_rol_long_zero(long val) {
+        // Count is known to be zero only after loop opts
+        int count = 42;
+        for (int i = 0; i < 4; i++) {
+            if ((i % 2) == 0) {
+                count = 0;
+            }
+        }
+        long res = Long.rotateLeft(val, count);
+        if (res != val) {
+            throw new RuntimeException("test_rol_long_zero failed: " + res + " != " + val);
+        }
+    }
+
+    public static void test_ror_int_zero(int val) {
+        // Count is known to be zero only after loop opts
+        int count = 42;
+        for (int i = 0; i < 4; i++) {
+            if ((i % 2) == 0) {
+                count = 0;
+            }
+        }
+        int res = Integer.rotateRight(val, count);
+        if (res != val) {
+            throw new RuntimeException("test_ror_int_zero failed: " + res + " != " + val);
+        }
+    }
+
+    public static void test_ror_long_zero(long val) {
+        // Count is known to be zero only after loop opts
+        int count = 42;
+        for (int i = 0; i < 4; i++) {
+            if ((i % 2) == 0) {
+                count = 0;
+            }
+        }
+        long res = Long.rotateRight(val, count);
+        if (res != val) {
+            throw new RuntimeException("test_ror_long_zero failed: " + res + " != " + val);
+        }
+    }
+
     public static void main(String args[]) throws Exception {
       rand = new Random(8248830);
 
@@ -300,6 +356,10 @@ public class TestRotate {
             test_rol_long_const(test_long[j], j);
             test_ror_long_const(test_long[j], j);
           }
+          test_rol_int_zero(i);
+          test_rol_long_zero(i);
+          test_ror_int_zero(i);
+          test_ror_long_zero(i);
         }
         System.out.println("test status : PASS");
       } catch (Exception e) {