Skip to content

Commit

Permalink
Require C++11 and use alignof() to fix alignment issues (#149)
Browse files Browse the repository at this point in the history
* Try requiring C11

* Implement `aligned_allocate()` and `aligned_void_deref()`

These allow using `Rf_allocVector()` to allocate a RAWSXP with some buffer padding, along with a dereferencing helper to get an aligned pointer to the block of memory

* Switch to C++11, since we know that is supported

This now uses a single C++11 compilation unit to get access to `alignof()`, which we call on the two types we need it for, `long double` and our custom `struct mean_state_t`. Forward declaring it in `summary-core.h` is enough to give us access to those C++ functions from C. We forward declare rather than including `summary-core-align.hpp`, as that will attempt to compile `extern "C"` with a C compiler, which won't work.

* NEWS bullet
  • Loading branch information
DavisVaughan authored Mar 19, 2021
1 parent 8cc244a commit 2e597d7
Show file tree
Hide file tree
Showing 13 changed files with 171 additions and 40 deletions.
1 change: 1 addition & 0 deletions DESCRIPTION
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ Encoding: UTF-8
LazyData: true
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.1.1
SystemRequirements: C++11
Collate:
'block.R'
'conditions.R'
Expand Down
3 changes: 3 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# slider (development version)

* Fixed a C alignment issue detected by CRAN's USBAN machine related to
allocating vectors of `long double`.

* Fixed a test that relied too strongly on the size of the C type,
`long double`, which can vary across platforms (#147).

Expand Down
38 changes: 38 additions & 0 deletions src/align.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#ifndef SLIDER_ALIGN_H
#define SLIDER_ALIGN_H

/*
* Following guidance of:
* https://stackoverflow.com/questions/227897/how-to-allocate-aligned-memory-only-using-the-standard-library
*
* 1) Allocate enough space to shift the pointer
* 2) Add to the pointer (p_x + buffer)
* 3) Round down to the closest boundary using `& mask`
*/

#include "slider.h"
#include <stdint.h> // uintptr_t

static
inline
SEXP
aligned_allocate(R_xlen_t n_elements,
size_t element_size,
size_t element_align) {
const size_t buffer = element_align - 1;
const R_xlen_t size = n_elements * element_size + buffer;
return Rf_allocVector(RAWSXP, size);
}

static
inline
void*
aligned_void_deref(SEXP x, size_t element_align) {
const size_t buffer = element_align - 1;
uintptr_t mask = ~ (uintptr_t)buffer;
uintptr_t p_x = (uintptr_t)RAW(x);
uintptr_t p_aligned = (p_x + buffer) & mask;
return (void*) p_aligned;
}

#endif
3 changes: 2 additions & 1 deletion src/segment-tree.c
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ struct segment_tree new_segment_tree(uint64_t n_leaves,
void (*state_finalize)(void* p_state, void* p_result),
void* (*nodes_increment)(void* p_nodes),
SEXP (*nodes_initialize)(uint64_t n),
void* (*nodes_void_deref)(SEXP nodes),
void (*aggregate_from_leaves)(const void* p_source, uint64_t begin, uint64_t end, void* p_dest),
void (*aggregate_from_nodes)(const void* p_source, uint64_t begin, uint64_t end, void* p_dest)) {
struct segment_tree tree;
Expand All @@ -33,7 +34,7 @@ struct segment_tree new_segment_tree(uint64_t n_leaves,
tree.p_p_level = (void**) RAW(tree.p_level);

tree.nodes = PROTECT(nodes_initialize(tree.n_nodes));
tree.p_nodes = r_deref(tree.nodes, TYPEOF(tree.nodes));
tree.p_nodes = nodes_void_deref(tree.nodes);

tree.state_reset = state_reset;
tree.state_finalize = state_finalize;
Expand Down
1 change: 1 addition & 0 deletions src/segment-tree.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ struct segment_tree new_segment_tree(uint64_t n_leaves,
void (*state_finalize)(void* p_state, void* p_result),
void* (*nodes_increment)(void* p_nodes),
SEXP (*nodes_initialize)(uint64_t n),
void* (*nodes_void_deref)(SEXP nodes),
void (*aggregate_from_leaves)(const void* p_source, uint64_t begin, uint64_t end, void* p_dest),
void (*aggregate_from_nodes)(const void* p_source, uint64_t begin, uint64_t end, void* p_dest));

Expand Down
21 changes: 21 additions & 0 deletions src/summary-core-align.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#include "summary-core-align.h"

extern "C" {

/*
* `alignof()` is C++11 specific, so this single compilation unit requires
* C++11, and we call these helpers from C in `summary-core.h`.
*
* Technically `alignof()` is also in C11, but it is unclear how well R supports
* that.
*/

size_t align_of_long_double() {
return alignof(long double);
}

size_t align_of_mean_state_t() {
return alignof(struct mean_state_t);
}

} // extern "C"
15 changes: 15 additions & 0 deletions src/summary-core-align.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#ifndef SLIDER_SUMMARY_CORE_ALIGN
#define SLIDER_SUMMARY_CORE_ALIGN

#include <stddef.h> // size_t

extern "C" {

#include "summary-core-types.h"

size_t align_of_long_double();
size_t align_of_mean_state_t();

} // extern "C"

#endif
11 changes: 11 additions & 0 deletions src/summary-core-types.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#ifndef SLIDER_SUMMARY_CORE_TYPES
#define SLIDER_SUMMARY_CORE_TYPES

#include <stdint.h> // uintptr_t

struct mean_state_t {
long double sum;
uint64_t count;
};

#endif
80 changes: 65 additions & 15 deletions src/summary-core.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@
#define SLIDER_SUMMARY_CORE

#include "slider.h"
#include "summary-core-types.h"
#include "align.h"

// From `summary-core-align.hpp`
size_t align_of_long_double();
size_t align_of_mean_state_t();

// -----------------------------------------------------------------------------
// Sum
Expand Down Expand Up @@ -30,9 +36,16 @@ static inline void* sum_nodes_increment(void* p_nodes) {
return (void*) (((long double*) p_nodes) + 1);
}

static inline void* sum_nodes_void_deref(SEXP nodes) {
return aligned_void_deref(nodes, align_of_long_double());
}
static inline long double* sum_nodes_deref(SEXP nodes) {
return (long double*) sum_nodes_void_deref(nodes);
}

static inline SEXP sum_nodes_initialize(uint64_t n) {
SEXP nodes = PROTECT(Rf_allocVector(RAWSXP, n * sizeof(long double)));
long double* p_nodes = (long double*) RAW(nodes);
SEXP nodes = PROTECT(aligned_allocate(n, sizeof(long double), align_of_long_double()));
long double* p_nodes = sum_nodes_deref(nodes);

for (uint64_t i = 0; i < n; ++i) {
p_nodes[i] = 0;
Expand Down Expand Up @@ -151,9 +164,16 @@ static inline void* prod_nodes_increment(void* p_nodes) {
return (void*) (((long double*) p_nodes) + 1);
}

static inline void* prod_nodes_void_deref(SEXP nodes) {
return aligned_void_deref(nodes, align_of_long_double());
}
static inline long double* prod_nodes_deref(SEXP nodes) {
return (long double*) prod_nodes_void_deref(nodes);
}

static inline SEXP prod_nodes_initialize(uint64_t n) {
SEXP nodes = PROTECT(Rf_allocVector(RAWSXP, n * sizeof(long double)));
long double* p_nodes = (long double*) RAW(nodes);
SEXP nodes = PROTECT(aligned_allocate(n, sizeof(long double), align_of_long_double()));
long double* p_nodes = prod_nodes_deref(nodes);

for (uint64_t i = 0; i < n; ++i) {
p_nodes[i] = 1;
Expand Down Expand Up @@ -248,11 +268,6 @@ static inline void prod_na_rm_aggregate_from_nodes(const void* p_source,
// -----------------------------------------------------------------------------
// Mean

struct mean_state_t {
long double sum;
uint64_t count;
};

static inline void mean_state_reset(void* p_state) {
struct mean_state_t* p_state_ = (struct mean_state_t*) p_state;
p_state_->sum = 0;
Expand All @@ -270,9 +285,16 @@ static inline void* mean_nodes_increment(void* p_nodes) {
return (void*) (((struct mean_state_t*) p_nodes) + 1);
}

static inline void* mean_nodes_void_deref(SEXP nodes) {
return aligned_void_deref(nodes, align_of_mean_state_t());
}
static inline struct mean_state_t* mean_nodes_deref(SEXP nodes) {
return (struct mean_state_t*) mean_nodes_void_deref(nodes);
}

static inline SEXP mean_nodes_initialize(uint64_t n) {
SEXP nodes = PROTECT(Rf_allocVector(RAWSXP, n * sizeof(struct mean_state_t)));
struct mean_state_t* p_nodes = (struct mean_state_t*) RAW(nodes);
SEXP nodes = PROTECT(aligned_allocate(n, sizeof(struct mean_state_t), align_of_mean_state_t()));
struct mean_state_t* p_nodes = mean_nodes_deref(nodes);

for (uint64_t i = 0; i < n; ++i) {
p_nodes[i].sum = 0;
Expand Down Expand Up @@ -388,9 +410,16 @@ static inline void* min_nodes_increment(void* p_nodes) {
return (void*) (((double*) p_nodes) + 1);
}

static inline double* min_nodes_deref(SEXP nodes) {
return REAL(nodes);
}
static inline void* min_nodes_void_deref(SEXP nodes) {
return (void*) min_nodes_deref(nodes);
}

static inline SEXP min_nodes_initialize(uint64_t n) {
SEXP nodes = PROTECT(Rf_allocVector(REALSXP, n));
double* p_nodes = REAL(nodes);
double* p_nodes = min_nodes_deref(nodes);

for (uint64_t i = 0; i < n; ++i) {
p_nodes[i] = R_PosInf;
Expand Down Expand Up @@ -473,9 +502,16 @@ static inline void* max_nodes_increment(void* p_nodes) {
return (void*) (((double*) p_nodes) + 1);
}

static inline double* max_nodes_deref(SEXP nodes) {
return REAL(nodes);
}
static inline void* max_nodes_void_deref(SEXP nodes) {
return (void*) max_nodes_deref(nodes);
}

static inline SEXP max_nodes_initialize(uint64_t n) {
SEXP nodes = PROTECT(Rf_allocVector(REALSXP, n));
double* p_nodes = REAL(nodes);
double* p_nodes = max_nodes_deref(nodes);

for (uint64_t i = 0; i < n; ++i) {
p_nodes[i] = R_NegInf;
Expand Down Expand Up @@ -558,9 +594,16 @@ static inline void* all_nodes_increment(void* p_nodes) {
return (void*) (((int*) p_nodes) + 1);
}

static inline int* all_nodes_deref(SEXP nodes) {
return LOGICAL(nodes);
}
static inline void* all_nodes_void_deref(SEXP nodes) {
return (void*) all_nodes_deref(nodes);
}

static inline SEXP all_nodes_initialize(uint64_t n) {
SEXP nodes = PROTECT(Rf_allocVector(LGLSXP, n));
int* p_nodes = LOGICAL(nodes);
int* p_nodes = all_nodes_deref(nodes);

for (uint64_t i = 0; i < n; ++i) {
p_nodes[i] = 1;
Expand Down Expand Up @@ -653,9 +696,16 @@ static inline void* any_nodes_increment(void* p_nodes) {
return (void*) (((int*) p_nodes) + 1);
}

static inline int* any_nodes_deref(SEXP nodes) {
return LOGICAL(nodes);
}
static inline void* any_nodes_void_deref(SEXP nodes) {
return (void*) any_nodes_deref(nodes);
}

static inline SEXP any_nodes_initialize(uint64_t n) {
SEXP nodes = PROTECT(Rf_allocVector(LGLSXP, n));
int* p_nodes = LOGICAL(nodes);
int* p_nodes = any_nodes_deref(nodes);

for (uint64_t i = 0; i < n; ++i) {
p_nodes[i] = 0;
Expand Down
7 changes: 7 additions & 0 deletions src/summary-index.c
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ static void slider_index_sum_core_impl(const double* p_x,
sum_state_finalize,
sum_nodes_increment,
sum_nodes_initialize,
sum_nodes_void_deref,
na_rm ? sum_na_rm_aggregate_from_leaves : sum_na_keep_aggregate_from_leaves,
na_rm ? sum_na_rm_aggregate_from_nodes : sum_na_keep_aggregate_from_nodes
);
Expand Down Expand Up @@ -300,6 +301,7 @@ static void slider_index_prod_core_impl(const double* p_x,
prod_state_finalize,
prod_nodes_increment,
prod_nodes_initialize,
prod_nodes_void_deref,
na_rm ? prod_na_rm_aggregate_from_leaves : prod_na_keep_aggregate_from_leaves,
na_rm ? prod_na_rm_aggregate_from_nodes : prod_na_keep_aggregate_from_nodes
);
Expand Down Expand Up @@ -384,6 +386,7 @@ static void slider_index_mean_core_impl(const double* p_x,
mean_state_finalize,
mean_nodes_increment,
mean_nodes_initialize,
mean_nodes_void_deref,
na_rm ? mean_na_rm_aggregate_from_leaves : mean_na_keep_aggregate_from_leaves,
na_rm ? mean_na_rm_aggregate_from_nodes : mean_na_keep_aggregate_from_nodes
);
Expand Down Expand Up @@ -468,6 +471,7 @@ static void slider_index_min_core_impl(const double* p_x,
min_state_finalize,
min_nodes_increment,
min_nodes_initialize,
min_nodes_void_deref,
na_rm ? min_na_rm_aggregate_from_leaves : min_na_keep_aggregate_from_leaves,
na_rm ? min_na_rm_aggregate_from_nodes : min_na_keep_aggregate_from_nodes
);
Expand Down Expand Up @@ -552,6 +556,7 @@ static void slider_index_max_core_impl(const double* p_x,
max_state_finalize,
max_nodes_increment,
max_nodes_initialize,
max_nodes_void_deref,
na_rm ? max_na_rm_aggregate_from_leaves : max_na_keep_aggregate_from_leaves,
na_rm ? max_na_rm_aggregate_from_nodes : max_na_keep_aggregate_from_nodes
);
Expand Down Expand Up @@ -636,6 +641,7 @@ static void slider_index_all_core_impl(const int* p_x,
all_state_finalize,
all_nodes_increment,
all_nodes_initialize,
all_nodes_void_deref,
na_rm ? all_na_rm_aggregate_from_leaves : all_na_keep_aggregate_from_leaves,
na_rm ? all_na_rm_aggregate_from_nodes : all_na_keep_aggregate_from_nodes
);
Expand Down Expand Up @@ -720,6 +726,7 @@ static void slider_index_any_core_impl(const int* p_x,
any_state_finalize,
any_nodes_increment,
any_nodes_initialize,
any_nodes_void_deref,
na_rm ? any_na_rm_aggregate_from_leaves : any_na_keep_aggregate_from_leaves,
na_rm ? any_na_rm_aggregate_from_nodes : any_na_keep_aggregate_from_nodes
);
Expand Down
7 changes: 7 additions & 0 deletions src/summary-slide.c
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ static inline void slide_sum_impl(const double* p_x,
sum_state_finalize,
sum_nodes_increment,
sum_nodes_initialize,
sum_nodes_void_deref,
na_rm ? sum_na_rm_aggregate_from_leaves : sum_na_keep_aggregate_from_leaves,
na_rm ? sum_na_rm_aggregate_from_nodes : sum_na_keep_aggregate_from_nodes
);
Expand Down Expand Up @@ -189,6 +190,7 @@ static inline void slide_prod_impl(const double* p_x,
prod_state_finalize,
prod_nodes_increment,
prod_nodes_initialize,
prod_nodes_void_deref,
na_rm ? prod_na_rm_aggregate_from_leaves : prod_na_keep_aggregate_from_leaves,
na_rm ? prod_na_rm_aggregate_from_nodes : prod_na_keep_aggregate_from_nodes
);
Expand Down Expand Up @@ -227,6 +229,7 @@ static inline void slide_mean_impl(const double* p_x,
mean_state_finalize,
mean_nodes_increment,
mean_nodes_initialize,
mean_nodes_void_deref,
na_rm ? mean_na_rm_aggregate_from_leaves : mean_na_keep_aggregate_from_leaves,
na_rm ? mean_na_rm_aggregate_from_nodes : mean_na_keep_aggregate_from_nodes
);
Expand Down Expand Up @@ -265,6 +268,7 @@ static inline void slide_min_impl(const double* p_x,
min_state_finalize,
min_nodes_increment,
min_nodes_initialize,
min_nodes_void_deref,
na_rm ? min_na_rm_aggregate_from_leaves : min_na_keep_aggregate_from_leaves,
na_rm ? min_na_rm_aggregate_from_nodes : min_na_keep_aggregate_from_nodes
);
Expand Down Expand Up @@ -303,6 +307,7 @@ static inline void slide_max_impl(const double* p_x,
max_state_finalize,
max_nodes_increment,
max_nodes_initialize,
max_nodes_void_deref,
na_rm ? max_na_rm_aggregate_from_leaves : max_na_keep_aggregate_from_leaves,
na_rm ? max_na_rm_aggregate_from_nodes : max_na_keep_aggregate_from_nodes
);
Expand Down Expand Up @@ -341,6 +346,7 @@ static inline void slide_all_impl(const int* p_x,
all_state_finalize,
all_nodes_increment,
all_nodes_initialize,
all_nodes_void_deref,
na_rm ? all_na_rm_aggregate_from_leaves : all_na_keep_aggregate_from_leaves,
na_rm ? all_na_rm_aggregate_from_nodes : all_na_keep_aggregate_from_nodes
);
Expand Down Expand Up @@ -379,6 +385,7 @@ static inline void slide_any_impl(const int* p_x,
any_state_finalize,
any_nodes_increment,
any_nodes_initialize,
any_nodes_void_deref,
na_rm ? any_na_rm_aggregate_from_leaves : any_na_keep_aggregate_from_leaves,
na_rm ? any_na_rm_aggregate_from_nodes : any_na_keep_aggregate_from_nodes
);
Expand Down
Loading

0 comments on commit 2e597d7

Please sign in to comment.