这是indexloc提供的服务，不要输入任何密码
Skip to content

Allow more cases to fall through to CHUNKMEMSET/CHUNKCOPY #1819

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Allow more cases to fall through to CHUNKMEMSET/CHUNKCOPY #1819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions arch/arm/chunkset_neon.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ typedef uint8x16_t chunk_t;
#define HAVE_CHUNKMEMSET_4
#define HAVE_CHUNKMEMSET_8
#define HAVE_CHUNK_MAG
#define MIN_CHUNKSIZE sizeof(chunk_t)

static const lut_rem_pair perm_idx_lut[13] = {
{0, 1}, /* 3 */
Expand Down
1 change: 1 addition & 0 deletions arch/generic/chunkset_c.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ typedef uint64_t chunk_t;

#define HAVE_CHUNKMEMSET_4
#define HAVE_CHUNKMEMSET_8
#define MIN_CHUNKSIZE sizeof(chunk_t)

static inline void chunkmemset_4(uint8_t *from, chunk_t *chunk) {
uint8_t *dest = (uint8_t *)chunk;
Expand Down
1 change: 1 addition & 0 deletions arch/power/chunkset_power8.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ typedef vector unsigned char chunk_t;
#define HAVE_CHUNKMEMSET_2
#define HAVE_CHUNKMEMSET_4
#define HAVE_CHUNKMEMSET_8
#define MIN_CHUNKSIZE sizeof(chunk_t)

static inline void chunkmemset_2(uint8_t *from, chunk_t *chunk) {
uint16_t tmp;
Expand Down
1 change: 1 addition & 0 deletions arch/riscv/chunkset_rvv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#define HAVE_CHUNKMEMSET_2
#define HAVE_CHUNKMEMSET_4
#define HAVE_CHUNKMEMSET_8
#define MIN_CHUNKSIZE CHUNK_SIZE
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 29, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Inconsistent chunk size definition needs attention

The RISC-V implementation defines CHUNK_SIZE as 32 and MIN_CHUNKSIZE as CHUNK_SIZE, but this differs from other architectures where:

  • Generic C: 8 bytes (uint64_t)
  • x86 (SSE2/SSSE3): 16 bytes (__m128i)
  • x86 (AVX2): 32 bytes (__m256i)
  • ARM NEON: 16 bytes (uint8x16_t)
  • POWER8: 16 bytes (vector unsigned char)

The RISC-V implementation uses a struct with 4 uint64_t (32 bytes) because "We don't have a 32-byte datatype for RISC-V arch". While the size is correctly matched to the struct, this inconsistency with other architectures' MIN_CHUNKSIZE definitions (which use sizeof(chunk_t)) should be reviewed.

🔗 Analysis chain

LGTM! Verify chunk size consistency.

The MIN_CHUNKSIZE macro definition correctly matches the CHUNK_SIZE of 32 bytes, which aligns with the chunk_t structure size (4 × uint64_t).

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify consistent chunk size definitions across architecture-specific files
rg -g 'arch/*/*.c' -g 'arch/*/*.h' "MIN_CHUNKSIZE|CHUNK_SIZE"

Length of output: 901

Script:

#!/bin/bash
# Let's verify the chunk_t definitions to understand the size variations
rg -g 'arch/*/*.h' -g 'arch/*/*.c' "typedef.*chunk_t" -A 3

Length of output: 1436

Script:

#!/bin/bash
# Let's check the RISC-V specific chunk_t definition and structure
rg -g 'arch/riscv/*' "chunk_t|struct chunk" -B 2 -A 4

Length of output: 3430


#define CHUNK_MEMSET_RVV_IMPL(elen) \
do { \
Expand Down
1 change: 1 addition & 0 deletions arch/x86/chunkset_avx2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ typedef __m128i halfchunk_t;
#define HAVE_CHUNKMEMSET_8
#define HAVE_CHUNKMEMSET_16
#define HAVE_CHUNKMEMSET_1
#define MIN_CHUNKSIZE sizeof(halfchunk_t)
#define HAVE_CHUNK_MAG
#define HAVE_HALF_CHUNK

Expand Down
1 change: 1 addition & 0 deletions arch/x86/chunkset_sse2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ typedef __m128i chunk_t;
#define HAVE_CHUNKMEMSET_2
#define HAVE_CHUNKMEMSET_4
#define HAVE_CHUNKMEMSET_8
#define MIN_CHUNKSIZE sizeof(chunk_t)
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 29, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

The MIN_CHUNKSIZE definition is inconsistent with AVX2/AVX512 implementations

The verification revealed that while MIN_CHUNKSIZE is consistently defined as sizeof(chunk_t) across architectures, the underlying chunk_t type varies:

  • SSE2/SSSE3: __m128i (16 bytes)
  • AVX2/AVX512: __m256i (32 bytes)
  • NEON: uint8x16_t (16 bytes)
  • POWER8: vector unsigned char (16 bytes)
  • Generic: uint64_t (8 bytes)

This inconsistency in chunk_t sizes means the SSE2 implementation's MIN_CHUNKSIZE (16 bytes) differs from AVX2/AVX512 (32 bytes), which could lead to suboptimal performance or incorrect behavior when switching between implementations.

  • Consider aligning MIN_CHUNKSIZE with the largest chunk size (32 bytes) across all x86 implementations
  • Review if the template code in chunkset_tpl.h correctly handles these size differences
🔗 Analysis chain

LGTM! The MIN_CHUNKSIZE definition is correct and well-placed.

The macro definition correctly uses sizeof(chunk_t) which evaluates to 16 bytes (size of __m128i). This aligns with SSE2's natural vector size and matches the PR's objective of standardizing chunk size handling across architectures.

Let's verify the consistency of MIN_CHUNKSIZE across other architecture files:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify MIN_CHUNKSIZE definition consistency across architecture files
# Expected: All architecture files should define MIN_CHUNKSIZE similarly

# Search for MIN_CHUNKSIZE definitions in architecture files
echo "Checking MIN_CHUNKSIZE definitions:"
rg "MIN_CHUNKSIZE.*sizeof\(chunk_t\)" arch/

# Verify chunk_t definitions to ensure correct sizes
echo -e "\nChecking chunk_t definitions:"
rg "typedef.*chunk_t" arch/

Length of output: 1037


static inline void chunkmemset_2(uint8_t *from, chunk_t *chunk) {
int16_t tmp;
Expand Down
1 change: 1 addition & 0 deletions arch/x86/chunkset_ssse3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ typedef __m128i chunk_t;
#define HAVE_CHUNKMEMSET_4
#define HAVE_CHUNKMEMSET_8
#define HAVE_CHUNK_MAG
#define MIN_CHUNKSIZE sizeof(chunk_t)

static const lut_rem_pair perm_idx_lut[13] = {
{0, 1}, /* 3 */
Expand Down
4 changes: 2 additions & 2 deletions chunkset_tpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ Z_INTERNAL uint8_t* CHUNKMEMSET_SAFE(uint8_t *out, uint8_t *from, unsigned len,
#endif

#ifndef HAVE_MASKED_READWRITE
if (UNLIKELY(left < sizeof(chunk_t))) {
if (UNLIKELY(left < MIN_CHUNKSIZE)) {
while (len > 0) {
*out++ = *from++;
--len;
Expand All @@ -276,7 +276,7 @@ static inline uint8_t *CHUNKCOPY_SAFE(uint8_t *out, uint8_t *from, uint64_t len,

#ifndef HAVE_MASKED_READWRITE
uint64_t from_dist = (uint64_t)llabs(safe - from);
if (UNLIKELY(from_dist < sizeof(chunk_t) || safelen < sizeof(chunk_t))) {
if (UNLIKELY(from_dist < MIN_CHUNKSIZE || safelen < MIN_CHUNKSIZE)) {
while (len--) {
*out++ = *from++;
}
Expand Down
Loading