Enable dynamic-sized RegisterBits (#515)

* Enable dynamic-sized RegisterBits * Revert "Enable dynamic-sized RegisterBits" This reverts commit a954891. * Keep 64B array but use heap when registers > 64B * Enable register bit tests * Downsize preallocated RegisterBits data space * Fixed smart pointer type * Fixed illegal access caused by illegal RegDef * Fixed uninitialized values * Remove large register from bad test * Moved CI forward; see what happens * Do not build helios anymore * Throw the exception in the initialze function --------- Co-authored-by: Knute Lingaard <[email protected]>
sparcians · Sep 9, 2024 · ce6b9dc · ce6b9dc
1 parent 5eaecbf
commit ce6b9dc
Show file tree

Hide file tree

Showing 10 changed files with 96 additions and 53 deletions.
diff --git a/.ci_support/linux_64_.yaml b/.ci_support/linux_64_.yaml
@@ -3,7 +3,7 @@ boost:
 c_compiler:
 - gcc
 c_compiler_version:
-- '12'
+- '13'
 cdt_name:
 - cos7
 channel_sources:
@@ -13,7 +13,7 @@ channel_targets:
 cxx_compiler:
 - gxx
 cxx_compiler_version:
-- '12'
+- '13'
 docker_image:
 - quay.io/condaforge/linux-anvil-cos7-x86_64
 hdf5:

diff --git a/.ci_support/osx_64_.yaml b/.ci_support/osx_64_.yaml
@@ -7,15 +7,15 @@ boost:
 c_compiler:
 - clang
 c_compiler_version:
-- '16'
+- '17'
 channel_sources:
 - conda-forge
 channel_targets:
 - conda-forge main
 cxx_compiler:
 - clangxx
 cxx_compiler_version:
-- '16'
+- '17'
 hdf5:
 - 1.14.3
 macos_machine:

diff --git a/.ci_support/osx_arm64_.yaml b/.ci_support/osx_arm64_.yaml
@@ -7,15 +7,15 @@ boost:
 c_compiler:
 - clang
 c_compiler_version:
-- '16'
+- '17'
 channel_sources:
 - conda-forge
 channel_targets:
 - conda-forge main
 cxx_compiler:
 - clangxx
 cxx_compiler_version:
-- '16'
+- '17'
 hdf5:
 - 1.14.3
 macos_machine:

diff --git a/README.md b/README.md
@@ -98,3 +98,7 @@ Install `conda smithy` instructions:
 conda install -n root -c conda-forge conda-smithy
 conda install -n root -c conda-forge conda-package-handling
 ```
+If `conda smithy` complains about being out of date:
+```
+conda update -n root conda-smithy
+```
diff --git a/conda.recipe/build.sh b/conda.recipe/build.sh
@@ -89,17 +89,17 @@ popd
 #  BUILD MAP/HELIOS
 #
 ################################################################################
-pushd helios
-mkdir -p release
-pushd release
-cmake -DCMAKE_BUILD_TYPE=Release \
-      -DCMAKE_INSTALL_PREFIX:PATH="$PREFIX" \
-      "${CMAKE_PLATFORM_FLAGS[@]}" \
-      ..
-cmake --build . -j "$CPU_COUNT" || cmake --build . -v
-popd
-popd
-
+#pushd helios
+#mkdir -p release
+#pushd release
+#cmake -DCMAKE_BUILD_TYPE=Release \
+#      -DCMAKE_INSTALL_PREFIX:PATH="$PREFIX" \
+#      "${CMAKE_PLATFORM_FLAGS[@]}" \
+#      ..
+#cmake --build . -j "$CPU_COUNT" || cmake --build . -v
+#popd
+#popd
+#
 ################################################################################
 #
 # Preserve build-phase test results so that we can track them individually

diff --git a/sparta/sparta/functional/Register.hpp b/sparta/sparta/functional/Register.hpp
@@ -1332,6 +1332,12 @@ class RegisterBase : public TreeNode
 private:
     RegisterBits computeWriteMask_(const Definition *def) const
     {
+        if(!isPowerOf2(def->bytes)) {
+            throw SpartaException("Register \"")
+                << getName() << "\" size in bytes must be a power of 2 larger than 0, is "
+                << def->bytes;
+        }
+
         const auto mask_size = def->bytes;
         RegisterBits write_mask(mask_size);
         RegisterBits partial_mask(mask_size);

diff --git a/sparta/sparta/functional/RegisterBits.hpp b/sparta/sparta/functional/RegisterBits.hpp
@@ -16,8 +16,8 @@ namespace sparta
      * \class RegisterBits
      *
      * This class is used in conjuntion with sparta::RegisterBase to
-     * quickly write masked registers of sizes between 1 and 512
-     * bytes.  This class replaces the use of BitArray.
+     * quickly write masked registers.  This class replaces the use
+     * of BitArray.
      *
      * The class works by assuming register data is handed to it via a
      * char array.  The class will "view" into this data until it's
@@ -63,9 +63,16 @@ namespace sparta
 
         // Copy the remote register data locally.
         void convert_() {
-            if(nullptr == local_data_) {
-                local_data_ = local_storage_.data();
-                ::memset(local_data_, 0, local_storage_.size());
+            if(nullptr == local_data_)
+            {
+                if(num_bytes_ <= local_storage_.size()) {
+                    local_data_ = local_storage_.data();
+                }
+                else {
+                    local_storage_alt_.reset(new uint8_t[num_bytes_]);
+                    local_data_ = local_storage_alt_.get();
+                }
+                ::memset(local_data_, 0, num_bytes_);
                 ::memcpy(local_data_, remote_data_, num_bytes_);
                 remote_data_ = local_data_;
             }
@@ -82,9 +89,12 @@ namespace sparta
             remote_data_(local_data_),
             num_bytes_(num_bytes)
         {
-            ::memset(local_data_, 0, local_storage_.size());
-            sparta_assert(num_bytes <= local_storage_.size(),
-                          "RegisterBits size is locked to 64 bytes. num_bytes requested: " << num_bytes);
+            if(num_bytes > local_storage_.size()) {
+                local_storage_alt_.reset(new uint8_t[num_bytes]);
+                local_data_ = local_storage_alt_.get();
+                remote_data_ = local_data_;
+            }
+            ::memset(local_data_, 0, num_bytes_);
         }
 
         /**
@@ -101,8 +111,12 @@ namespace sparta
             remote_data_(local_data_),
             num_bytes_(num_bytes)
         {
-            sparta_assert(num_bytes <= local_storage_.size(),
-                          "RegisterBits size is locked to 64 bytes. num_bytes requested: " << num_bytes);
+            if(num_bytes > local_storage_.size()) {
+                local_storage_alt_.reset(new uint8_t[num_bytes]);
+                local_data_ = local_storage_alt_.get();
+                remote_data_ = local_data_;
+            }
+            ::memset(local_data_, 0, num_bytes_);
             sparta_assert(sizeof(DataT) <= num_bytes);
             set(data);
         }
@@ -118,10 +132,7 @@ namespace sparta
             local_data_(data_ptr),
             remote_data_(local_data_),
             num_bytes_(num_bytes)
-        {
-            sparta_assert(num_bytes <= local_storage_.size(),
-                          "RegisterBits size is locked to 64 bytes. num_bytes requested: " << num_bytes);
-        }
+        {}
 
         /**
          * \brief Create a class pointing into the given data constantly, of the given size
@@ -133,10 +144,7 @@ namespace sparta
         RegisterBits(const uint8_t * data, const size_t num_bytes) :
             remote_data_(data),
             num_bytes_(num_bytes)
-        {
-            sparta_assert(num_bytes <= local_storage_.size(),
-                          "RegisterBits size is locked to 64 bytes. num_bytes requested: " << num_bytes);
-        }
+        {}
 
         /**
          * \brief Create a nullptr version of the data.  This would be an invalid class
@@ -150,10 +158,22 @@ namespace sparta
          * If the original is pointing to its own memory, that will be copied
          */
         RegisterBits(const RegisterBits & orig) :
-            local_storage_(orig.local_storage_),
-            local_data_(orig.local_data_ == nullptr ? nullptr : local_storage_.data()),
-            remote_data_(orig.local_data_ == orig.remote_data_ ? local_data_ : orig.remote_data_)
-        {}
+            num_bytes_(orig.num_bytes_)
+        {
+            if(num_bytes_ <= local_storage_.size()) {
+                local_storage_ = orig.local_storage_;
+                local_data_ = orig.local_data_ == nullptr ? nullptr : local_storage_.data();
+            }
+            else if (orig.local_data_ == nullptr) {
+                local_data_ = nullptr;
+            }
+            else {
+                local_storage_alt_.reset(new uint8_t[orig.getSize()]);
+                local_data_ = local_storage_alt_.get();
+                ::memcpy(local_data_, orig.local_data_, num_bytes_);
+            }
+            remote_data_ = orig.local_data_ == orig.remote_data_ ? local_data_ : orig.remote_data_;
+        }
 
         /**
          * \brief Move
@@ -163,10 +183,19 @@ namespace sparta
          * will be moved.  The original is nullified.
          */
         RegisterBits(RegisterBits && orig) :
-            local_storage_(std::move(orig.local_storage_)),
             num_bytes_(orig.num_bytes_)
         {
-            local_data_  = (orig.local_data_ == nullptr ? nullptr : local_storage_.data());
+            if(num_bytes_ <= local_storage_.size()) {
+                local_storage_ = std::move(orig.local_storage_);
+                local_data_  = (orig.local_data_ == nullptr ? nullptr : local_storage_.data());
+            }
+            else if (orig.local_data_ == nullptr) {
+                local_data_ = nullptr;
+            }
+            else {
+                local_storage_alt_ = std::move(orig.local_storage_alt_);
+                local_data_ = local_storage_alt_.get();
+            }
             remote_data_ = (orig.local_data_ == orig.remote_data_ ? local_data_ : orig.remote_data_);
             orig.local_data_ = nullptr;
             orig.remote_data_ = nullptr;
@@ -191,7 +220,7 @@ namespace sparta
                 // 64-bit chunks
                 for(uint32_t idx = 0; idx < num_bytes_; idx += 8)
                 {
-                    *reinterpret_cast<uint64_t*>(final_value.local_storage_.data() + idx) =
+                    *reinterpret_cast<uint64_t*>(final_value.local_data_ + idx) =
                         *reinterpret_cast<const uint64_t*>(remote_data_ + idx) |
                         *reinterpret_cast<const uint64_t*>(rh_bits.remote_data_ + idx);
                 }
@@ -227,7 +256,7 @@ namespace sparta
                 // 64-bit chunks
                 for(uint32_t idx = 0; idx < num_bytes_; idx += 8)
                 {
-                    *reinterpret_cast<uint64_t*>(final_value.local_storage_.data() + idx) =
+                    *reinterpret_cast<uint64_t*>(final_value.local_data_ + idx) =
                         *reinterpret_cast<const uint64_t*>(remote_data_ + idx) &
                         *reinterpret_cast<const uint64_t*>(rh_bits.remote_data_ + idx);
                 }
@@ -262,7 +291,7 @@ namespace sparta
                 // 64-bit compares
                 for(uint32_t idx = 0; idx < num_bytes_; idx += 8)
                 {
-                    *reinterpret_cast<uint64_t*>(final_value.local_storage_.data() + idx) =
+                    *reinterpret_cast<uint64_t*>(final_value.local_data_ + idx) =
                         ~*reinterpret_cast<const uint64_t*>(remote_data_ + idx);
                 }
                 return final_value;
@@ -293,7 +322,7 @@ namespace sparta
             {
                 RegisterBits final_value(num_bytes_);
                 const uint64_t * src_data = reinterpret_cast<const uint64_t*>(remote_data_);
-                uint64_t *     final_data = reinterpret_cast<uint64_t*>(final_value.local_storage_.data());
+                uint64_t *     final_data = reinterpret_cast<uint64_t*>(final_value.local_data_);
                 const uint32_t num_dbl_words = num_bytes_ / 8;
 
                 // Determine the number of double words that will be shifted
@@ -385,7 +414,7 @@ namespace sparta
             {
                 RegisterBits final_value(num_bytes_);
                 const uint64_t * src_data = reinterpret_cast<const uint64_t*>(remote_data_);
-                uint64_t *     final_data = reinterpret_cast<uint64_t*>(final_value.local_storage_.data());
+                uint64_t *     final_data = reinterpret_cast<uint64_t*>(final_value.local_data_);
                 const uint32_t num_dbl_words = num_bytes_ / 8;
 
                 // Determine the number of double words that will be shifted
@@ -630,7 +659,7 @@ namespace sparta
          */
         void fill(const uint8_t fill_data) {
             convert_();
-            local_storage_.fill(fill_data);
+            ::memset(local_data_, fill_data, num_bytes_);
         }
 
         /**
@@ -697,9 +726,10 @@ namespace sparta
 
     private:
 
-        std::array<uint8_t, 64> local_storage_; //!< Local storage
-        uint8_t               * local_data_  = nullptr; //!< Points to null if using remote data
-        const uint8_t         * remote_data_ = nullptr; //!< Remove data; points to local_data_ if no remote
-        const uint64_t          num_bytes_ = 0; //!< Number of bytse
+        std::array<uint8_t, 8>     local_storage_; //!< Local storage
+        std::unique_ptr<uint8_t[]> local_storage_alt_; //!< Alternative local storage when register size > 64B
+        uint8_t                  * local_data_  = nullptr; //!< Points to null if using remote data
+        const uint8_t            * remote_data_ = nullptr; //!< Remove data; points to local_data_ if no remote
+        const uint64_t             num_bytes_ = 0; //!< Number of bytse
     };
 }
diff --git a/sparta/test/Register/CMakeLists.txt b/sparta/test/Register/CMakeLists.txt
@@ -3,3 +3,7 @@ project(Register_test)
 sparta_add_test_executable(Register_test Register_test.cpp)
 
 sparta_test(Register_test Register_test_RUN)
+
+sparta_add_test_executable(RegisterBits_test reg_bit_test.cpp)
+
+sparta_test(RegisterBits_test RegisterBits_test_RUN)
diff --git a/sparta/test/Register/Register_test.cpp b/sparta/test/Register/Register_test.cpp
@@ -323,7 +323,6 @@ void testBadRegs()
         3,    // non-power-of-2-count regs not allowed
         5,    // non-power-of-2-count regs not allowed
         9,    // Just to prove that odd-byte-count regs are rejected; not just primes
-        8192  // 8192 Bytes per register is very likely too large
     };
 
     // Test each separately because ALL sizes must fail!

diff --git a/sparta/test/Register/reg_bit_test.cpp b/sparta/test/Register/reg_bit_test.cpp
@@ -1,5 +1,5 @@
 
-#include "RegisterBits.hpp"
+#include "sparta/functional/Register.hpp"
 #include <iostream>
 #include <iomanip>