Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable dynamic-sized RegisterBits #515

Merged
merged 13 commits into from
Sep 9, 2024
4 changes: 2 additions & 2 deletions .ci_support/linux_64_.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ boost:
c_compiler:
- gcc
c_compiler_version:
- '12'
- '13'
cdt_name:
- cos7
channel_sources:
Expand All @@ -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:
Expand Down
4 changes: 2 additions & 2 deletions .ci_support/osx_64_.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
4 changes: 2 additions & 2 deletions .ci_support/osx_arm64_.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
```
22 changes: 11 additions & 11 deletions conda.recipe/build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions sparta/sparta/functional/Register.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
98 changes: 64 additions & 34 deletions sparta/sparta/functional/RegisterBits.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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_;
}
Expand All @@ -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_);
}

/**
Expand All @@ -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);
}
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does this happen?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, when can num_bytes_ be <= local storage size and the original still have its local_data_ pointer set to nullptr?

Copy link
Member Author

@furuame furuame Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does this happen?

Actually I don’t know 😂 I just follow the same logics but take care of the original storage and alternative storage here.

Curious, when can num_bytes_ be <= local storage size and the original still have its local_data_ pointer set to nullptr?

There can be a scenario that RegisterBits is initialized by a raw pointer, which could be nullptr. That’s the only case I can think of.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if it has > 0 bytes, will it ever be nullptr?

Copy link
Member Author

@furuame furuame Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, local_data_ could be nullptr if RegisterBits is initialized by a const uint8 pointer. In that case, only remote_data_ pointer is initialized. local_data_ is nullptr until the value is changed.

}
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
Expand All @@ -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;
Expand All @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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_);
}

/**
Expand Down Expand Up @@ -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
};
}
4 changes: 4 additions & 0 deletions sparta/test/Register/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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)
1 change: 0 additions & 1 deletion sparta/test/Register/Register_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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!
Expand Down
2 changes: 1 addition & 1 deletion sparta/test/Register/reg_bit_test.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@

#include "RegisterBits.hpp"
#include "sparta/functional/Register.hpp"
#include <iostream>
#include <iomanip>

Expand Down
Loading