From 1c4c99eae2d0eeb8747c5922b1cd9c179da0a7f0 Mon Sep 17 00:00:00 2001 From: Tobias Hartmann Date: Tue, 24 Nov 2020 16:52:13 +0000 Subject: [PATCH] 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 | 4 +- src/hotspot/share/opto/addnode.cpp | 26 +-- src/hotspot/share/opto/mulnode.cpp | 151 ++++++++++++------ src/hotspot/share/opto/mulnode.hpp | 2 + .../jtreg/compiler/intrinsics/TestRotate.java | 62 ++++++- 5 files changed, 180 insertions(+), 65 deletions(-) diff --git a/src/hotspot/cpu/x86/assembler_x86.cpp b/src/hotspot/cpu/x86/assembler_x86.cpp index a5981a204c4..9fb3c9cc539 100644 --- a/src/hotspot/cpu/x86/assembler_x86.cpp +++ b/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)); diff --git a/src/hotspot/share/opto/addnode.cpp b/src/hotspot/share/opto/addnode.cpp index b220bbcfa1a..1e4891dce4e 100644 --- a/src/hotspot/share/opto/addnode.cpp +++ b/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* rshift = in(2)->in(2); - Node* shift = rotate_shift(phase, lshift, rshift, 0x1F); - if (shift != NULL) { - return new RotateLeftNode(in(1)->in(1), shift, TypeInt::INT); - } - return NULL; + Node* lshift = in(1)->in(2); + Node* rshift = in(2)->in(2); + Node* shift = rotate_shift(phase, lshift, rshift, 0x1F); + if (shift != NULL) { + return new RotateLeftNode(in(1)->in(1), shift, TypeInt::INT); + } + 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 *lshift = in(2)->in(2); - Node* shift = rotate_shift(phase, rshift, lshift, 0x1F); - if (shift != NULL) { - return new RotateRightNode(in(1)->in(1), shift, TypeInt::INT); - } + Node* rshift = in(1)->in(2); + Node* lshift = in(2)->in(2); + Node* shift = rotate_shift(phase, rshift, lshift, 0x1F); + if (shift != NULL) { + return new RotateRightNode(in(1)->in(1), shift, TypeInt::INT); + } } return NULL; } diff --git a/src/hotspot/share/opto/mulnode.cpp b/src/hotspot/share/opto/mulnode.cpp index 32961e5b0ed..3bdfd45bb0d 100644 --- a/src/hotspot/share/opto/mulnode.cpp +++ b/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) { - const Type *t = phase->type(shiftNode->in(2)); - if (t == Type::TOP) return retVal; // Right input is dead. - const TypeInt *t2 = t->isa_int(); - if (!t2 || !t2->is_con()) return retVal; // Right input is a constant. - - return t2->get_con(); +static bool const_shift_count(PhaseGVN* phase, Node* shiftNode, int* count) { + const TypeInt* tcount = phase->type(shiftNode->in(2))->isa_int(); + if (tcount != NULL && tcount->is_con()) { + *count = tcount->get_con(); + return true; + } + return false; } -static int maskShiftAmount(PhaseGVN *phase, Node *shiftNode, int nBits) { - int shift = getShiftCon(phase, shiftNode, 0); - int maskedShift = shift & (nBits - 1); - - if (maskedShift == 0) return 0; // Let Identity() handle 0 shift count. +static int maskShiftAmount(PhaseGVN* phase, Node* shiftNode, int nBits) { + int count = 0; + if (const_shift_count(phase, shiftNode, &count)) { + int maskedShift = count & (nBits - 1); + if (maskedShift == 0) { + // Let Identity() handle 0 shift count. + return 0; + } - if (shift != 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); + 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 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); - if (shift == -1) return this; - if ((shift & (BitsPerJavaInteger - 1)) == 0) return in(1); - - // Check for useless sign-masking - if (in(1)->Opcode() == Op_LShiftI && - in(1)->req() == 3 && - in(1)->in(2) == in(2)) { - shift &= BitsPerJavaInteger-1; // semantics of Java shifts - // Compute masks for which this shifting doesn't change - int lo = (-1 << (BitsPerJavaInteger - ((uint)shift)-1)); // FFFF8000 - int hi = ~lo; // 00007FFF - const TypeInt *t11 = phase->type(in(1)->in(1))->isa_int(); - if (!t11) 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 + int count = 0; + if (const_shift_count(phase, this, &count)) { + if ((count & (BitsPerJavaInteger - 1)) == 0) { + // Shift by a multiple of 32 does nothing + return in(1); + } + // Check for useless sign-masking + if (in(1)->Opcode() == Op_LShiftI && + in(1)->req() == 3 && + in(1)->in(2) == in(2)) { + count &= BitsPerJavaInteger-1; // semantics of Java shifts + // Compute masks for which this shifting doesn't change + int lo = (-1 << (BitsPerJavaInteger - ((uint)count)-1)); // FFFF8000 + int hi = ~lo; // 00007FFF + const TypeInt* t11 = phase->type(in(1)->in(1))->isa_int(); + if (t11 == NULL) { + 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); - if ((shift & (BitsPerJavaInteger - 1)) == 0) return in(1); + 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); + } // 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; } diff --git a/src/hotspot/share/opto/mulnode.hpp b/src/hotspot/share/opto/mulnode.hpp index 16481f9a1ee..e1005865c62 100644 --- a/src/hotspot/share/opto/mulnode.hpp +++ b/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; }; diff --git a/test/hotspot/jtreg/compiler/intrinsics/TestRotate.java b/test/hotspot/jtreg/compiler/intrinsics/TestRotate.java index 0bbc33b1406..5f00fc5fb95 100644 --- a/test/hotspot/jtreg/compiler/intrinsics/TestRotate.java +++ b/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 static void test_ror_long_const(long val, int index) { 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 static void main(String args[]) throws Exception { 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) {