From ce6b9dcf9cbbaf6932155d171eb66f448ceb18c3 Mon Sep 17 00:00:00 2001 From: Zen Date: Mon, 9 Sep 2024 12:59:49 +0800 Subject: [PATCH] Enable dynamic-sized RegisterBits (#515) * Enable dynamic-sized RegisterBits * Revert "Enable dynamic-sized RegisterBits" This reverts commit a954891aadca604768cd1be8435eb4a63f55e469. * 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 --- .ci_support/linux_64_.yaml | 4 +- .ci_support/osx_64_.yaml | 4 +- .ci_support/osx_arm64_.yaml | 4 +- README.md | 4 + conda.recipe/build.sh | 22 ++--- sparta/sparta/functional/Register.hpp | 6 ++ sparta/sparta/functional/RegisterBits.hpp | 98 +++++++++++++++-------- sparta/test/Register/CMakeLists.txt | 4 + sparta/test/Register/Register_test.cpp | 1 - sparta/test/Register/reg_bit_test.cpp | 2 +- 10 files changed, 96 insertions(+), 53 deletions(-) diff --git a/.ci_support/linux_64_.yaml b/.ci_support/linux_64_.yaml index 97800b3a9b..3aacb735f4 100644 --- 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 index f61f880be0..5b8851063d 100644 --- a/.ci_support/osx_64_.yaml +++ b/.ci_support/osx_64_.yaml @@ -7,7 +7,7 @@ boost: c_compiler: - clang c_compiler_version: -- '16' +- '17' channel_sources: - conda-forge channel_targets: @@ -15,7 +15,7 @@ channel_targets: 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 index 24940654a1..31881fcccc 100644 --- a/.ci_support/osx_arm64_.yaml +++ b/.ci_support/osx_arm64_.yaml @@ -7,7 +7,7 @@ boost: c_compiler: - clang c_compiler_version: -- '16' +- '17' channel_sources: - conda-forge channel_targets: @@ -15,7 +15,7 @@ channel_targets: cxx_compiler: - clangxx cxx_compiler_version: -- '16' +- '17' hdf5: - 1.14.3 macos_machine: diff --git a/README.md b/README.md index 8a623bb6f6..53b16ca7f9 100644 --- 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 index 46c3efd88d..5c149c9263 100644 --- 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 index 3b30a00be7..a4816595b7 100644 --- 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 index 4e5a210ccb..1db515af8d 100644 --- 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(final_value.local_storage_.data() + idx) = + *reinterpret_cast(final_value.local_data_ + idx) = *reinterpret_cast(remote_data_ + idx) | *reinterpret_cast(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(final_value.local_storage_.data() + idx) = + *reinterpret_cast(final_value.local_data_ + idx) = *reinterpret_cast(remote_data_ + idx) & *reinterpret_cast(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(final_value.local_storage_.data() + idx) = + *reinterpret_cast(final_value.local_data_ + idx) = ~*reinterpret_cast(remote_data_ + idx); } return final_value; @@ -293,7 +322,7 @@ namespace sparta { RegisterBits final_value(num_bytes_); const uint64_t * src_data = reinterpret_cast(remote_data_); - uint64_t * final_data = reinterpret_cast(final_value.local_storage_.data()); + uint64_t * final_data = reinterpret_cast(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(remote_data_); - uint64_t * final_data = reinterpret_cast(final_value.local_storage_.data()); + uint64_t * final_data = reinterpret_cast(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 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 local_storage_; //!< Local storage + std::unique_ptr 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 index 3de2d345fe..6a9d663984 100644 --- 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 index 0d64d24e92..276cd13533 100644 --- 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 index bc7e9e62e6..c72f139b0b 100644 --- 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 #include