Skip to content

Commit e092c01

Browse files
Fix alignment issues in synchronization_common.h (halide#6272)
* Fix alignment issues in synchronization_common.h To work around old COMDAT issues, we allocated the table as a char array and cast it to what we want; unfortunately this doesn't guarantee the right alignment for the table and in some environments (eg wasm) we can get unaligned-access failures. We could fix this by forcing the right alignment, but since we fixed COMDAT issues in another way a while back (adding smarts to LLVM_Runtime_Linker), let's just remove the hack and declare it normally. Also added some drive-by changes to ensure that the hashtable size and HASH_TABLE_BITS were safe (this happened to be the case before but wasn't enforced), and also to init all the fields in hash_bucket. (Q: do we really need `check_hash()` to exist? With the mods in place above, is it possible for addr_hash() to return a bad index?) * Always use HASH_TABLE_BITS in addr_hash() * Only use check_hash() in DEBUG_RUNTIME builds
1 parent 4307645 commit e092c01

File tree

1 file changed

+25
-20
lines changed

1 file changed

+25
-20
lines changed

src/runtime/synchronization_common.h

Lines changed: 25 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -458,29 +458,23 @@ struct queue_data {
458458
};
459459

460460
// Must be a power of two.
461-
#define LOAD_FACTOR 4
461+
constexpr int LOAD_FACTOR = 4;
462462

463463
struct hash_bucket {
464464
word_lock mutex;
465465

466-
queue_data *head; // Is this queue_data or thread_data?
467-
queue_data *tail; // Is this queue_data or thread_data?
466+
queue_data *head = nullptr; // Is this queue_data or thread_data?
467+
queue_data *tail = nullptr; // Is this queue_data or thread_data?
468468
};
469469

470-
// The use of a #define here and table_storage is because if
471-
// a class with a constructor is used, clang generates a COMDAT
472-
// which cannot be lowered for Mac OS due to MachO. A better
473-
// solution is desired of course.
474-
#define HASH_TABLE_BITS 10
470+
constexpr int HASH_TABLE_SIZE = MAX_THREADS * LOAD_FACTOR;
475471
struct hash_table {
476-
hash_bucket buckets[MAX_THREADS * LOAD_FACTOR];
472+
hash_bucket buckets[HASH_TABLE_SIZE];
477473
};
478-
WEAK char table_storage[sizeof(hash_table)];
479-
#define table (*(hash_table *)table_storage)
474+
WEAK hash_table table;
480475

481-
ALWAYS_INLINE void check_hash(uintptr_t hash) {
482-
halide_assert(nullptr, hash < sizeof(table.buckets) / sizeof(table.buckets[0]));
483-
}
476+
constexpr int HASH_TABLE_BITS = 10;
477+
static_assert((1 << HASH_TABLE_BITS) >= MAX_THREADS * LOAD_FACTOR);
484478

485479
#if 0
486480
WEAK void dump_hash() {
@@ -496,20 +490,29 @@ WEAK void dump_hash() {
496490
}
497491
#endif
498492

499-
static ALWAYS_INLINE uintptr_t addr_hash(uintptr_t addr, uint32_t bits) {
493+
static ALWAYS_INLINE uintptr_t addr_hash(uintptr_t addr) {
500494
// Fibonacci hashing. The golden ratio is 1.9E3779B97F4A7C15F39...
501495
// in hexadecimal.
502496
if (sizeof(uintptr_t) >= 8) {
503-
return (addr * (uintptr_t)0x9E3779B97F4A7C15) >> (64 - bits);
497+
return (addr * (uintptr_t)0x9E3779B97F4A7C15) >> (64 - HASH_TABLE_BITS);
504498
} else {
505-
return (addr * (uintptr_t)0x9E3779B9) >> (32 - bits);
499+
return (addr * (uintptr_t)0x9E3779B9) >> (32 - HASH_TABLE_BITS);
506500
}
507501
}
508502

503+
#ifdef DEBUG_RUNTIME
504+
// Any hash calculated by addr_hash() should be incapable of being outside this range.
505+
ALWAYS_INLINE void check_hash(uintptr_t hash) {
506+
halide_assert(nullptr, hash < HASH_TABLE_SIZE);
507+
}
508+
#endif // DEBUG_RUNTIME
509+
509510
WEAK hash_bucket &lock_bucket(uintptr_t addr) {
510-
uintptr_t hash = addr_hash(addr, HASH_TABLE_BITS);
511+
uintptr_t hash = addr_hash(addr);
511512

513+
#ifdef DEBUG_RUNTIME
512514
check_hash(hash);
515+
#endif
513516

514517
// TODO: if resizing is implemented, loop, etc.
515518
hash_bucket &bucket = table.buckets[hash];
@@ -530,11 +533,13 @@ struct bucket_pair {
530533

531534
WEAK bucket_pair lock_bucket_pair(uintptr_t addr_from, uintptr_t addr_to) {
532535
// TODO: if resizing is implemented, loop, etc.
533-
uintptr_t hash_from = addr_hash(addr_from, HASH_TABLE_BITS);
534-
uintptr_t hash_to = addr_hash(addr_to, HASH_TABLE_BITS);
536+
uintptr_t hash_from = addr_hash(addr_from);
537+
uintptr_t hash_to = addr_hash(addr_to);
535538

539+
#ifdef DEBUG_RUNTIME
536540
check_hash(hash_from);
537541
check_hash(hash_to);
542+
#endif
538543

539544
// Lock the bucket with the smaller hash first in order
540545
// to prevent deadlock.

0 commit comments

Comments
 (0)