flac dev - Dec 2015 - about word size in bitreader/bitwriter

There are preprocessor definitions in bitreader.c and bitwriter.c:

     /* Things should be fastest when this matches the machine word size */
     /* WATCHOUT: if you change this you must also change the following #defines
down to SWAP_BE_WORD_TO_HOST below to match */
     /* WATCHOUT: there are a few places where the code will not work unless
uint32_t is >= 32 bits wide */
     #define FLAC__BYTES_PER_WORD 4
     #define FLAC__BITS_PER_WORD 32
     #define FLAC__WORD_ALL_ONES ((FLAC__uint32)0xffffffff)
     ...

and the following code in bitreader.c:

     #if FLAC__BYTES_PER_WORD == 4
         switch(br->crc16_align) {
             case  0: crc = FLAC__CRC16_UPDATE((unsigned)(word >> 24),
crc);
             case  8: crc = FLAC__CRC16_UPDATE((unsigned)((word >> 16)
& 0xff), crc);
             case 16: crc = FLAC__CRC16_UPDATE((unsigned)((word >> 8)
& 0xff), crc);
             case 24: br->read_crc16 = FLAC__CRC16_UPDATE((unsigned)(word
& 0xff), crc);
         }
     #elif FLAC__BYTES_PER_WORD == 8
         switch(br->crc16_align) {
             case  0: crc = FLAC__CRC16_UPDATE((unsigned)(word >> 56),
crc);
             case  8: crc = FLAC__CRC16_UPDATE((unsigned)((word >> 48)
& 0xff), crc);
             case 16: crc = FLAC__CRC16_UPDATE((unsigned)((word >> 40)
& 0xff), crc);
             case 24: crc = FLAC__CRC16_UPDATE((unsigned)((word >> 32)
& 0xff), crc);
             case 32: crc = FLAC__CRC16_UPDATE((unsigned)((word >> 24)
& 0xff), crc);
             case 40: crc = FLAC__CRC16_UPDATE((unsigned)((word >> 16)
& 0xff), crc);
             case 48: crc = FLAC__CRC16_UPDATE((unsigned)((word >> 8)
& 0xff), crc);
             case 56: br->read_crc16 = FLAC__CRC16_UPDATE((unsigned)(word
& 0xff), crc);
         }
     #else
     ...


This implies that there was intention to set FLAC__BYTES_PER_WORD to 8 on 64-bit
architectures. However currently the code in bitreader.c/bitwriter.c doesn't
care
much about 64-bit words: there are places like

     const FLAC__uint32 mask2 = FLAC__WORD_ALL_ONES >> (31-parameter);

and FLAC__bitwriter_write_raw_uint64() simply calls
FLAC__bitwriter_write_raw_uint32() twice...


And there were three patches --

<http://git.xiph.org/?p=flac.git;a=commitdiff;h=66bd44bacc28cb154b5c9349cf3288da9ec74501>
<http://git.xiph.org/?p=flac.git;a=commitdiff;h=387b72731dbf79450050987b9b36bf70f286b098>
<http://git.xiph.org/?p=flac.git;a=commitdiff;h=d30fe60fc616d626df2336e91bc698677a732447>

that replaced bitness-agnostic brword/bwword and COUNT_ZERO_MSBS() with uint32_t
and
FLAC__clz_uint32()  (which are apparently 32-bit only).


Does it make sense to restore the ability to use FLAC__BYTES_PER_WORD == 8?
(Honestly, I'm not sure that it ever worked previously... and I'm not
sure that
this change can improve libFLAC performance).

Or is it better to change the comment that implies that it's possible?

lvqcl wrote:
> Does it make sense to restore the ability to use FLAC__BYTES_PER_WORD == 8?
> (Honestly, I'm not sure that it ever worked previously...
I'm actually pretty sure that it never worked.
> and I'm not sure that
> this change can improve libFLAC performance).
I'll take your word on that.
> Or is it better to change the comment that implies that it's possible?
The think in and ideal world we would a:

* Make it work correctly FLAC__BYTES_PER_WORD == 8 and compare the performance
  with FLAC__BYTES_PER_WORD == 4.
* If there is an statistically measurable performance, keep it, otherwise
  remove the FLAC__BYTES_PER_WORD == 8 code all together.

Since you seem to think there little to be gainned, maybe its better to
just remove the FLAC__BYTES_PER_WORD == 8 code now.

Erik
-- 
----------------------------------------------------------------------
Erik de Castro Lopo
http://www.mega-nerd.com/

Erik de Castro Lopo wrote:
> The think in and ideal world we would a:
>
> * Make it work correctly FLAC__BYTES_PER_WORD == 8 and compare the
performance
>   with FLAC__BYTES_PER_WORD == 4.
> * If there is an statistically measurable performance, keep it, otherwise
>   remove the FLAC__BYTES_PER_WORD == 8 code all together.
I'll try to do it, but I don't have a deep understanding of
bit(read|write)
routines such as FLAC__bitreader_read_rice_signed_block() and other.
Maybe Miroslav Lichvar can say something?

> Since you seem to think there little to be gainned
No, I just don't know what performance improvement can be obtained
(and of course FLAC is very fast already).