Clean up U8TO*_LE macro implementations

[mattst88]

The code guarded by #ifndef U32_ALIGNMENT_REQUIRED attempts to optimize
byte-swapping by doing unaligned loads, but accessing data through
unaligned pointers is undefined behavior in C. Moreover, compilers are
more than capable of recognizing these open-coded byte-swap patterns and
emitting a bswap instruction, or an unaligned load instruction, or a
combined load, etc. There's no need for multiple paths to attain the
desired result.

The same commit went into perl in commit e8864dba8095 but with a typo in
U8TO64_LE. See Perl/perl5#17249

See https://rt.perl.org/Ticket/Display.html?id=133495
See Perl/perl5#17244

[demerphq]

I dont think this patch is correct. And I dont think the patch to perl is correct. Here is part of the patch: -#ifndef U32_ALIGNMENT_REQUIRED +#ifndef U8TO16_LE #if (BYTEORDER == 0x1234 || BYTEORDER == 0x12345678) - #define U8TO16_LE(ptr) (*((const U16*)(ptr))) - #define U8TO32_LE(ptr) (*((const U32*)(ptr))) - #define U8TO64_LE(ptr) (*((const U64*)(ptr))) + #define U8TO16_LE(ptr) ((U32)(ptr)[1]|(U32)(ptr)[0]<<8) + #define U8TO32_LE(ptr) ((U32)(ptr)[3]|(U32)(ptr)[2]<<8|(U32)(ptr)[1]<<16|(U32)(ptr)[0]<<24) + #define U8TO64_LE(ptr) ((U64)(ptr)[7]|(U64)(ptr)[6]<<8|(U64)(ptr)[5]<<16|(U64)(ptr)[4]<<24|\ + (U64)(ptr)[3]<<32|(U64)(ptr)[4]<<40|\ + (U64)(ptr)[1]<<48|(U64)(ptr)[0]<<56) #elif (BYTEORDER == 0x4321 || BYTEORDER == 0x87654321) - #if defined(__GNUC__) && (__GNUC__>4 || (__GNUC__==4 && __GNUC_MINOR__>=3)) - #define U8TO16_LE(ptr) (__builtin_bswap16(*((U16*)(ptr)))) - #define U8TO32_LE(ptr) (__builtin_bswap32(*((U32*)(ptr)))) - #define U8TO64_LE(ptr) (__builtin_bswap64(*((U64*)(ptr)))) - #endif + #define U8TO16_LE(ptr) ((U32)(ptr)[0]|(U32)(ptr)[1]<<8) + #define U8TO32_LE(ptr) ((U32)(ptr)[0]|(U32)(ptr)[1]<<8|(U32)(ptr)[2]<<16|(U32)(ptr)[3]<<24) + #define U8TO64_LE(ptr) ((U64)(ptr)[0]|(U64)(ptr)[1]<<8|(U64)(ptr)[2]<<16|(U64)(ptr)[3]<<24|\ + (U64)(ptr)[4]<<32|(U64)(ptr)[5]<<40|\ + (U64)(ptr)[6]<<48|(U64)(ptr)[7]<<56) #endif #endif This change is backwards. Consider this part of the change: - #define U8TO16_LE(ptr) (*((const U16*)(ptr))) + #define U8TO16_LE(ptr) ((U32)(ptr)[1]|(U32)(ptr)[0]<<8) The correct change is: - #define U8TO16_LE(ptr) (*((const U16*)(ptr))) + #define U8TO16_LE(ptr) ((U32)(ptr)[0]|(U32)(ptr)[1]<<8) The patch to Perl needs to be reverted or reversed, and the patch to Algorithm::MinPerfHash also. Yves

…

On Mon, 4 Nov 2019 at 01:18, Matt Turner ***@***.***> wrote: The code guarded by #ifndef U32_ALIGNMENT_REQUIRED attempts to optimize byte-swapping by doing unaligned loads, but accessing data through unaligned pointers is undefined behavior in C. Moreover, compilers are more than capable of recognizing these open-coded byte-swap patterns and emitting a bswap instruction, or an unaligned load instruction, or a combined load, etc. There's no need for multiple paths to attain the desired result. The same commit went into perl in commit e8864dba8095 but with a typo in U8TO64_LE. See Perl/perl5#17249 See https://rt.perl.org/Ticket/Display.html?id=133495 See Perl/perl5#17244 ________________________________ You can view, comment on, or merge this pull request online at: #8 Commit Summary Clean up U8TO*_LE macro implementations File Changes M mph_hv_macro.h (31) Patch Links: https://github.com/demerphq/Algorithm-MinPerfHashTwoLevel/pull/8.patch https://github.com/demerphq/Algorithm-MinPerfHashTwoLevel/pull/8.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

-- perl -Mre=debug -e "/just|another|perl|hacker/"

[mattst88]

Ugh. Thank you for the review. I've corrected the patch now.