Re: [Tarantool-patches] [PATCH v2 04/10] crc32: align memory access

["Timur Safin" <tsafin@tarantool.org>]

Yup, this is better

LGTM

: -----Original Message-----
: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Sent: Friday, May 29, 2020 2:23 AM
: To: Timur Safin <tsafin@tarantool.org>; tarantool-
: patches@dev.tarantool.org; alyapunov@tarantool.org; korablev@tarantool.org
: Subject: Re: [PATCH v2 04/10] crc32: align memory access
: 
: Thanks for the review!
: 
: On 28/05/2020 22:11, Timur Safin wrote:
: >
: >
: > : -----Original Message-----
: > : From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: > : Subject: [PATCH v2 04/10] crc32: align memory access
: > :
: > :
: > : diff --git a/src/cpu_feature.c b/src/cpu_feature.c
: > : index 98567ccb3..9bf6223de 100644
: > : --- a/src/cpu_feature.c
: > : +++ b/src/cpu_feature.c
: > : @@ -50,7 +51,7 @@
: > :
: > :
: > :  static uint32_t
: > : -crc32c_hw_byte(uint32_t crc, unsigned char const *data, unsigned int
: > : length)
: > : +crc32c_hw_byte(uint32_t crc, char const *data, unsigned int length)
: > :  {
: > :  	while (length--) {
: > :  		__asm__ __volatile__(
: > : @@ -68,6 +69,26 @@ crc32c_hw_byte(uint32_t crc, unsigned char const
: *data,
: > : unsigned int length)
: > :  uint32_t
: > :  crc32c_hw(uint32_t crc, const char *buf, unsigned int len)
: > :  {
: > : +	const int align = alignof(unsigned long);
: > : +	unsigned long addr = (unsigned long)buf;
: > : +	unsigned int not_aligned_prefix =
: > : +		((addr - 1 + align) & ~(align - 1)) - addr;
: >
: > Hmm, hmm...
: >
: > Isn't it simple `addr % align`? Or even `addr & (align - 1)` ?
: 
: Consider the example: addr = 14, align = 8.
: Then not_aligned_prefix = 2. Need to read first 2
: bytes one by one to get to 16, the closest aligned
: address.
: 
: addr % align = 14 % 8 = 6 != 2
: addr & (align - 1) = 14 & 7 = 1110 & 0111 = 110 = 6 != 2
: 
: But yeah, this could be done simpler: align - addr % align.
: This will give how many bytes are needed to the next
: aligned address. I wrote the solution above by blindly using
: 'aligned - not aligned' and the same schema as in
: small_align().
: 
: Here is the diff:
: 
: ====================
: diff --git a/src/cpu_feature.c b/src/cpu_feature.c
: index 9bf6223de..7b284fa98 100644
: --- a/src/cpu_feature.c
: +++ b/src/cpu_feature.c
: @@ -69,10 +69,8 @@ crc32c_hw_byte(uint32_t crc, char const *data, unsigned
: int length)
:  uint32_t
:  crc32c_hw(uint32_t crc, const char *buf, unsigned int len)
:  {
: -	const int align = alignof(unsigned long);
: -	unsigned long addr = (unsigned long)buf;
: -	unsigned int not_aligned_prefix =
: -		((addr - 1 + align) & ~(align - 1)) - addr;
: +	const unsigned int align = alignof(unsigned long);
: +	unsigned int not_aligned_prefix = align - (unsigned long)buf %
: align;
:  	/*
:  	 * Calculate CRC32 for the prefix byte-by-byte so as to
:  	 * then use aligned words to calculate the rest. This is