8257069: C2: Clarify and sanity test RegMask/RegMaskIterator

Reviewed-by: jvernee, kvn

Author: cl4es

src/hotspot/share/adlc/output_h.cpp

@@ -98,7 +98,7 @@ void ArchDesc::buildMachRegisterNumbers(FILE *fp_hpp) {
   }
 
   fprintf(fp_hpp, "\n// Size of register-mask in ints\n");
-  fprintf(fp_hpp, "#define RM_SIZE %d\n",RegisterForm::RegMask_Size());
+  fprintf(fp_hpp, "#define RM_SIZE %d\n", RegisterForm::RegMask_Size());
   fprintf(fp_hpp, "// Unroll factor for loops over the data in a RegMask\n");
   fprintf(fp_hpp, "#define FORALL_BODY ");
   int len = RegisterForm::RegMask_Size();

src/hotspot/share/opto/regmask.hpp

@@ -58,11 +58,13 @@ class RegMask {
 
   friend class RegMaskIterator;
 
-  enum {
-    _WordBits    = BitsPerWord,
-    _LogWordBits = LogBitsPerWord,
-    _RM_SIZE     = LP64_ONLY(align_up(RM_SIZE, 2) >> 1) NOT_LP64(RM_SIZE)
-  };
+  // The RM_SIZE is aligned to 64-bit - assert that this holds
+  LP64_ONLY(STATIC_ASSERT(is_aligned(RM_SIZE, 2)));
+
+  static const unsigned int _WordBitMask = BitsPerWord - 1U;
+  static const unsigned int _LogWordBits = LogBitsPerWord;
+  static const unsigned int _RM_SIZE     = LP64_ONLY(RM_SIZE >> 1) NOT_LP64(RM_SIZE);
+  static const unsigned int _RM_MAX      = _RM_SIZE - 1U;
 
   union {
     // Array of Register Mask bits.  This array is large enough to cover
@@ -82,7 +84,7 @@ class RegMask {
   unsigned int _hwm;
 
  public:
-  enum { CHUNK_SIZE = RM_SIZE*BitsPerInt };
+  enum { CHUNK_SIZE = _RM_SIZE * BitsPerWord };
 
   // SlotsPerLong is 2, since slots are 32 bits and longs are 64 bits.
   // Also, consider the maximum alignment size for a normally allocated
@@ -121,7 +123,7 @@ class RegMask {
     FORALL_BODY
 #   undef BODY
     _lwm = 0;
-    _hwm = _RM_SIZE - 1;
+    _hwm = _RM_MAX;
     while (_hwm > 0      && _RM_UP[_hwm] == 0) _hwm--;
     while ((_lwm < _hwm) && _RM_UP[_lwm] == 0) _lwm++;
     assert(valid_watermarks(), "post-condition");
@@ -138,7 +140,7 @@ class RegMask {
   }
 
   // Construct an empty mask
-  RegMask() : _RM_UP(), _lwm(_RM_SIZE - 1), _hwm(0) {
+  RegMask() : _RM_UP(), _lwm(_RM_MAX), _hwm(0) {
     assert(valid_watermarks(), "post-condition");
   }
 
@@ -152,15 +154,19 @@ class RegMask {
     assert(reg < CHUNK_SIZE, "");
 
     unsigned r = (unsigned)reg;
-    return _RM_UP[r >> _LogWordBits] & (uintptr_t(1) <<(r & (_WordBits - 1U)));
+    return _RM_UP[r >> _LogWordBits] & (uintptr_t(1) << (r & _WordBitMask));
   }
 
   // The last bit in the register mask indicates that the mask should repeat
   // indefinitely with ONE bits.  Returns TRUE if mask is infinite or
   // unbounded in size.  Returns FALSE if mask is finite size.
-  bool is_AllStack() const { return _RM_UP[_RM_SIZE - 1U] >> (_WordBits - 1U); }
+  bool is_AllStack() const {
+    return (_RM_UP[_RM_MAX] & (uintptr_t(1) << _WordBitMask)) != 0;
+  }
 
-  void set_AllStack() { Insert(OptoReg::Name(CHUNK_SIZE-1)); }
+  void set_AllStack() {
+    _RM_UP[_RM_MAX] |= (uintptr_t(1) << _WordBitMask);
+  }
 
   // Test for being a not-empty mask.
   bool is_NotEmpty() const {
@@ -178,7 +184,7 @@ class RegMask {
     for (unsigned i = _lwm; i <= _hwm; i++) {
       uintptr_t bits = _RM_UP[i];
       if (bits) {
-        return OptoReg::Name((i<<_LogWordBits) + find_lowest_bit(bits));
+        return OptoReg::Name((i << _LogWordBits) + find_lowest_bit(bits));
       }
     }
     return OptoReg::Name(OptoReg::Bad);
@@ -192,7 +198,7 @@ class RegMask {
     while (i > _lwm) {
       uintptr_t bits = _RM_UP[--i];
       if (bits) {
-        return OptoReg::Name((i<<_LogWordBits) + find_highest_bit(bits));
+        return OptoReg::Name((i << _LogWordBits) + find_highest_bit(bits));
       }
     }
     return OptoReg::Name(OptoReg::Bad);
@@ -270,17 +276,17 @@ class RegMask {
 
   // Clear a register mask
   void Clear() {
-    _lwm = _RM_SIZE - 1;
+    _lwm = _RM_MAX;
     _hwm = 0;
-    memset(_RM_UP, 0, sizeof(uintptr_t)*_RM_SIZE);
+    memset(_RM_UP, 0, sizeof(uintptr_t) * _RM_SIZE);
     assert(valid_watermarks(), "sanity");
   }
 
   // Fill a register mask with 1's
   void Set_All() {
     _lwm = 0;
-    _hwm = _RM_SIZE - 1;
-    memset(_RM_UP, 0xFF, sizeof(uintptr_t)*_RM_SIZE);
+    _hwm = _RM_MAX;
+    memset(_RM_UP, 0xFF, sizeof(uintptr_t) * _RM_SIZE);
     assert(valid_watermarks(), "sanity");
   }
 
@@ -294,15 +300,15 @@ class RegMask {
     unsigned index = r >> _LogWordBits;
     if (index > _hwm) _hwm = index;
     if (index < _lwm) _lwm = index;
-    _RM_UP[index] |= (uintptr_t(1) << (r & (_WordBits - 1U)));
+    _RM_UP[index] |= (uintptr_t(1) << (r & _WordBitMask));
     assert(valid_watermarks(), "post-condition");
   }
 
   // Remove register from mask
   void Remove(OptoReg::Name reg) {
     assert(reg < CHUNK_SIZE, "");
     unsigned r = (unsigned)reg;
-    _RM_UP[r >> _LogWordBits] &= ~(uintptr_t(1) << (r & (_WordBits-1U)));
+    _RM_UP[r >> _LogWordBits] &= ~(uintptr_t(1) << (r & _WordBitMask));
   }
 
   // OR 'rm' into 'this'
@@ -355,23 +361,23 @@ class RegMask {
     // NOTE: -1 in computation reflects the usage of the last
     //       bit of the regmask as an infinite stack flag and
     //       -7 is to keep mask aligned for largest value (VecZ).
-    return (int)reg < (int)(CHUNK_SIZE-1);
+    return (int)reg < (int)(CHUNK_SIZE - 1);
   }
   static bool can_represent_arg(OptoReg::Name reg) {
     // NOTE: -SlotsPerVecZ in computation reflects the need
     //       to keep mask aligned for largest value (VecZ).
-    return (int)reg < (int)(CHUNK_SIZE-SlotsPerVecZ);
+    return (int)reg < (int)(CHUNK_SIZE - SlotsPerVecZ);
   }
 };
 
 class RegMaskIterator {
  private:
-  uintptr_t _current_word;
+  uintptr_t _current_bits;
   unsigned int _next_index;
   OptoReg::Name _reg;
-  const RegMask&  _rm;
+  const RegMask& _rm;
  public:
-  RegMaskIterator(const RegMask& rm) : _current_word(0), _next_index(rm._lwm), _reg(OptoReg::Special), _rm(rm) {
+  RegMaskIterator(const RegMask& rm) : _current_bits(0), _next_index(rm._lwm), _reg(OptoReg::Bad), _rm(rm) {
     // Calculate the first element
     next();
   }
@@ -383,26 +389,44 @@ class RegMaskIterator {
   // Get the current element and calculate the next
   OptoReg::Name next() {
     OptoReg::Name r = _reg;
-    if (_current_word != 0) {
-      unsigned int next_bit = find_lowest_bit(_current_word);
+
+    // This bit shift scheme, borrowed from IndexSetIterator,
+    // shifts the _current_bits down by the number of trailing
+    // zeros - which leaves the "current" bit on position zero,
+    // then subtracts by 1 to clear it. This quirk avoids the
+    // undefined behavior that could arise if trying to shift
+    // away the bit with a single >> (next_bit + 1) shift when
+    // next_bit is 31/63. It also keeps number of shifts and
+    // arithmetic ops to a minimum.
+
+    // We have previously found bits at _next_index - 1, and
+    // still have some left at the same index.
+    if (_current_bits != 0) {
+      unsigned int next_bit = find_lowest_bit(_current_bits);
+      assert(_reg != OptoReg::Bad, "can't be in a bad state");
       assert(next_bit > 0, "must be");
-      assert(((_current_word >> next_bit) & 0x1) == 1, "sanity");
-      _current_word = (_current_word >> next_bit) - 1;
+      assert(((_current_bits >> next_bit) & 0x1) == 1, "lowest bit must be set after shift");
+      _current_bits = (_current_bits >> next_bit) - 1;
       _reg = OptoReg::add(_reg, next_bit);
       return r;
     }
 
+    // Find the next word with bits
     while (_next_index <= _rm._hwm) {
-      _current_word = _rm._RM_UP[_next_index++];
-      if (_current_word != 0) {
-        unsigned int next_bit = find_lowest_bit(_current_word);
-        assert(((_current_word >> next_bit) & 0x1) == 1, "sanity");
-        _current_word = (_current_word >> next_bit) - 1;
+      _current_bits = _rm._RM_UP[_next_index++];
+      if (_current_bits != 0) {
+        // Found a word. Calculate the first register element and
+        // prepare _current_bits by shifting it down and clearing
+        // the lowest bit
+        unsigned int next_bit = find_lowest_bit(_current_bits);
+        assert(((_current_bits >> next_bit) & 0x1) == 1, "lowest bit must be set after shift");
+        _current_bits = (_current_bits >> next_bit) - 1;
         _reg = OptoReg::Name(((_next_index - 1) << RegMask::_LogWordBits) + next_bit);
         return r;
       }
     }
 
+    // No more bits
     _reg = OptoReg::Name(OptoReg::Bad);
     return r;
   }

test/hotspot/gtest/opto/test_regmask.cpp

@@ -0,0 +1,156 @@
+/*
+ * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ *
+ */
+
+#include "precompiled.hpp"
+#include "opto/regmask.hpp"
+#include "unittest.hpp"
+
+// Sanity tests for RegMask and RegMaskIterator
+
+static void contains_expected_num_of_registers(const RegMask& rm, unsigned int expected) {
+
+  ASSERT_TRUE(rm.Size() == expected);
+  if (expected > 0) {
+    ASSERT_TRUE(rm.is_NotEmpty());
+  } else {
+    ASSERT_TRUE(!rm.is_NotEmpty());
+    ASSERT_TRUE(!rm.is_AllStack());
+  }
+
+  RegMaskIterator rmi(rm);
+  unsigned int count = 0;
+  OptoReg::Name reg = OptoReg::Bad;
+  while (rmi.has_next()) {
+    reg = rmi.next();
+    ASSERT_TRUE(OptoReg::is_valid(reg));
+    count++;
+  }
+  ASSERT_EQ(OptoReg::Bad, rmi.next());
+  ASSERT_TRUE(count == expected);
+}
+
+TEST_VM(RegMask, empty) {
+  RegMask rm;
+  contains_expected_num_of_registers(rm, 0);
+}
+
+TEST_VM(RegMask, iteration) {
+  RegMask rm;
+  rm.Insert(30);
+  rm.Insert(31);
+  rm.Insert(32);
+  rm.Insert(33);
+  rm.Insert(62);
+  rm.Insert(63);
+  rm.Insert(64);
+  rm.Insert(65);
+
+  RegMaskIterator rmi(rm);
+  ASSERT_TRUE(rmi.next() == OptoReg::Name(30));
+  ASSERT_TRUE(rmi.next() == OptoReg::Name(31));
+  ASSERT_TRUE(rmi.next() == OptoReg::Name(32));
+  ASSERT_TRUE(rmi.next() == OptoReg::Name(33));
+  ASSERT_TRUE(rmi.next() == OptoReg::Name(62));
+  ASSERT_TRUE(rmi.next() == OptoReg::Name(63));
+  ASSERT_TRUE(rmi.next() == OptoReg::Name(64));
+  ASSERT_TRUE(rmi.next() == OptoReg::Name(65));
+  ASSERT_FALSE(rmi.has_next());
+}
+
+TEST_VM(RegMask, Set_ALL) {
+  // Check that Set_All doesn't add bits outside of CHUNK_SIZE
+  RegMask rm;
+  rm.Set_All();
+  ASSERT_TRUE(rm.Size() == RegMask::CHUNK_SIZE);
+  ASSERT_TRUE(rm.is_NotEmpty());
+  // Set_All sets AllStack bit
+  ASSERT_TRUE(rm.is_AllStack());
+  contains_expected_num_of_registers(rm, RegMask::CHUNK_SIZE);
+}
+
+TEST_VM(RegMask, Clear) {
+  // Check that Clear doesn't leave any stray bits
+  RegMask rm;
+  rm.Set_All();
+  rm.Clear();
+  contains_expected_num_of_registers(rm, 0);
+}
+
+TEST_VM(RegMask, AND) {
+  RegMask rm1;
+  rm1.Insert(OptoReg::Name(1));
+  contains_expected_num_of_registers(rm1, 1);
+  ASSERT_TRUE(rm1.Member(OptoReg::Name(1)));
+
+  rm1.AND(rm1);
+  contains_expected_num_of_registers(rm1, 1);
+
+  RegMask rm2;
+  rm1.AND(rm2);
+  contains_expected_num_of_registers(rm1, 0);
+  contains_expected_num_of_registers(rm2, 0);
+}
+
+TEST_VM(RegMask, OR) {
+  RegMask rm1;
+  rm1.Insert(OptoReg::Name(1));
+  contains_expected_num_of_registers(rm1, 1);
+  ASSERT_TRUE(rm1.Member(OptoReg::Name(1)));
+
+  rm1.OR(rm1);
+  contains_expected_num_of_registers(rm1, 1);
+
+  RegMask rm2;
+  rm1.OR(rm2);
+  contains_expected_num_of_registers(rm1, 1);
+  contains_expected_num_of_registers(rm2, 0);
+}
+
+TEST_VM(RegMask, SUBTRACT) {
+  RegMask rm1;
+  RegMask rm2;
+
+  rm2.Set_All();
+  for (int i = 17; i < RegMask::CHUNK_SIZE; i++) {
+    rm1.Insert(i);
+  }
+  ASSERT_TRUE(rm1.is_AllStack());
+  rm2.SUBTRACT(rm1);
+  contains_expected_num_of_registers(rm1, RegMask::CHUNK_SIZE - 17);
+  contains_expected_num_of_registers(rm2, 17);
+}
+
+TEST_VM(RegMask, is_bound1) {
+  RegMask rm;
+  ASSERT_FALSE(rm.is_bound1());
+  for (int i = 0; i < RegMask::CHUNK_SIZE - 1; i++) {
+    rm.Insert(i);
+    ASSERT_TRUE(rm.is_bound1());
+    contains_expected_num_of_registers(rm, 1);
+    rm.Remove(i);
+  }
+  // AllStack bit does not count as a bound register
+  rm.set_AllStack();
+  ASSERT_FALSE(rm.is_bound1());
+}