Re: Proposal for enabling auto-vectorization for checksum calculations - Mailing list pgsql-hackers
From | Oleg Tselebrovskiy |
---|---|
Subject | Re: Proposal for enabling auto-vectorization for checksum calculations |
Date | |
Msg-id | d023881ae837e0e116d8645c89e319da@postgrespro.ru Whole thread Raw |
In response to | Re: Proposal for enabling auto-vectorization for checksum calculations (Andrew Kim <tenistarkim@gmail.com>) |
Responses |
Re: Proposal for enabling auto-vectorization for checksum calculations
|
List | pgsql-hackers |
Greetings! I've also tried to use AVX2 to speedup checksums and I've found your approach quite interesting But I see some issues with v6 patch 1) checksum.c was moved to src/port, but special meson rules are left in src/backend/storage/page/meson.build. As a result, assembly code for moved src/port/checksum.c doesn't use -funroll-loops and -ftree-vectorize (latter isn't probably needed now, due to the nature of the patch). The same is true for src/port/Makefile, there are no instructions to use CFLAGS_UNROLL_LOOPS and CFLAGS_VECTORIZE 2) checksum.c was moved to src/port, but checksum.h and checksum_impl.h are left in src/include/storage. I think they both should be moved to src/include/port, as John Naylor suggested in his review of v5 3) checksum_impl.h now doesn't provide any code, so including it in external programs won't allow checksum calculation. I think that all code should be in checksum_impl.h, and external programs could just define USE_AVX2_WITH_RUNTIME_CHECK (probably using similar checks as we are) to use AVX2 implementation. If not - then they will default to default realisation 4) I don't understand why do we need to check for AVX2 intrinsics if we don't use those in code (at least I don't see them directly)? As in review of v5, couldn't test functions in configure, config/c-compiler.m4 and ./meson.build just be {return 0;} or {return 1;}? 5) Why do we need both src/backend/storage/page/checksum.c and src/port/checksum.c? 6) > +/* Function declarations for ISA-specific implementations */ > +uint32 pg_checksum_block_default(const PGChecksummablePage *page); > +#ifdef USE_AVX2_WITH_RUNTIME_CHECK > +uint32 pg_checksum_block_avx2(const PGChecksummablePage *page); > +#endif What is "ISA-specific implementations" in this comment? Maybe I'm just not familiar with the term? Or is it an artifact from macro implementation? 7) Why remove all comments from code of pg_checksum_block_default? I could understand if you just removed comments from pg_checksum_block_avx2, since it just duplicates code (though I personally would leave all the comments even when duplicating code), but I don't understand removing comments from pg_checksum_block_default 8) It might be a personal taste, but pg_checksum_block_dispatch looks more like "choose" function from src/port/pg_crc32c_sse42_choose.c and alike. "dispatch" from src/include/port/pg_crc32c looks a little different - we don't choose function pointer once there, we choose between inlined computation and calling a function with runtime check. So I'd suggest changing name of pg_checksum_block_dispatch to pg_checksum_block_choose Other than those, I think the core of this patch is good Oleg Tselebrovskiy, PostgresPro
pgsql-hackers by date: