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

Buffers stores data and meta #283

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

lamphamsy
Copy link
Contributor

Buffers consists of a vector of n DATA buffers (array of T) and an OPTIONAL vector of n META buffers (array of uint8_t). A meta buffer corresponds to a data buffer. Every s-byte data element of a data buffer corresponds to an s-bit element of the meta buffer. A pair of data and meta elements can
represent an integer of 8*s + s-bits Each data element points to a buffer of size m * sizeof(T) bytes.
Hence, each meta element points to a buffer of size bsize = m * sizeof(T) / 8 bytes that should be an integer, i.e. m * sizeof(T) % 8 = 0. This condition is not difficult to achieve as a convenient size can be always chosen with a negligible impact on its application.

- declare mem_len as size_t
- remove useless operations separating even & odd parts
- remove useless set operation that
A vector of `n` DATA buffers (array of T) and an OPTIONAL vector of `n`
META buffers (array of `uint8_t`). A meta buffer corresponds to a data
buffer. Every `s`-byte data element of a data buffer corresponds to a
`s`-bit element of the meta buffer. A pair of data and meta elements can
represent an integer of `8*s + s`-bits
Each data element points to a buffer of size `m * sizeof(T)` bytes.
Hence, each meta element points to a buffer of size
`bsize = m * sizeof(T) / 8` bytes that should be an integer, i.e.
`m * sizeof(T) % 8 = 0`. This condition is not difficult to achieved as
a convenient `size` can be always chosen with a negligible impact on its
application.
- use vector to store reversed bit numbers
- use Buffer's function for the prepare step of FFT and inverse FFT
- Add `set_meta(buf_id, ele_id, val)` setting meta of a given element
- Add `set(buf_id, ele_id, lo, hi)` setting both of data and meta at the
`ele_id`th of the `buf_id`th buffer/meta, according to a given unpacked
value represented by its two half parts `hi` and `lo`.
template <typename T>
inline T mask(size_t len)
{
return ((static_cast<T>(1) << len) - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could have an assert to ensure that len is not greater than the number of bit in T.
Optional: We may also want to check that T is an unsigned type

@@ -52,6 +52,24 @@ class Buffers;
template <typename T>
bool operator==(const Buffers<T>& lhs, const Buffers<T>& rhs);

// Return a mask of `len` ones
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be a proper Doxygen comment, same for set_bits.

return ((static_cast<T>(1) << len) - 1);
}

// Set a range of bits of `dest` according to bits of `src`
Copy link
Contributor

Choose a reason for hiding this comment

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

Not so clear, could be written as Copy bits_nbbits starting atd_beginfromsrcintodst`.

Also each parameter could be documented, using Doxygen (and bits_nb could be renamed len and d_begin to offset).

// reset dest
dest &= ~(m << d_begin);
// set dest
dest |= (static_cast<uint8_t>(src & m) << d_begin);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why casting to uint8_t? this looks buggy to me.

In any case, lossy cast like here should use narrow_cast instead of static_cast.

@@ -141,9 +159,11 @@ class Buffers final {
T* get(int i);
const T* get(int i) const;
void get(int buf_id, size_t ele_id, T& hi, T& lo) const;
void set(int buf_id, size_t ele_id, T hi, T lo);
Copy link
Contributor

Choose a reason for hiding this comment

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

ele_id could be called index or idx, no? (same for set_meta).


const size_t bytes = s * sizeof(T);
size_t r = bytes / CHAR_BIT;
while (r * CHAR_BIT < bytes) {
Copy link
Contributor

Choose a reason for hiding this comment

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

see my previous comment on that while.


const size_t bytes = m_size * CHAR_BIT;
size_t r = bytes / sizeof(T);
while (r * sizeof(T) < bytes) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto about the while

* @param m_size - given meta size, in bytes
* @return meta size
*/
static size_t compute_size(size_t m_size)
Copy link
Contributor

Choose a reason for hiding this comment

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

compute_size and compute_meta_size are too similar, you should have a single function instead of copy/pasting.

};

/** Calculate conventional size of buffers
* Conentional size is the lowest number of words that is at least a given
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Conentional/conventional/

Also, skip a line between the short description (the first line) and the detailed one (the rest).

* @param size_alignment - alignment number of output size, in bytes
* @param meta_size_alignment - alignment number of meta size according to
* the out size, in byte
* @return conventional size, in words
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the size of word? is it 32-bit or is it sizeof(T)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants