insertions(+), 517 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index bf6a030e2c..8066ef3047 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2025-02-17 Paul Eggert
+
+ crc-tests: use consistent type for randomb
+ * tests/randomb.c (randomb): Now array of char, not
On 2025-01-18 Sam Russell wrote:
> IIRC we test values from 0-16 bytes in the unit test and the pclmul
> implementation doesn't get hit for smaller values?
Correct. lib/crc.c has
if (pclmul_enabled && len >= 16)
return crc32_update_no_xor_pclmul (crc, buf, len);
y licensing...
>
> Lasse Collin writes:
>
> > I tested Gnulib's lib/crc-x86_64-pclmul.c (v1.0-1387-g5dd298feb0) a
> > bit. I changed the target attribute to plain "pclmul" to make it work
> > on a processor that lacks AVX. With 0-3 byte inputs the result differs
>
Thanks for discussion, Lasse. I suppose the current situation is
difficult to change due to mainly licensing...
Lasse Collin writes:
> I tested Gnulib's lib/crc-x86_64-pclmul.c (v1.0-1387-g5dd298feb0) a
> bit. I changed the target attribute to plain "pclmul" to make it
your implementation that we don't have. And then
> use it in xz?
My code hasn't been included in any xz release yet. I don't want to
backport it to 5.6.x and 5.7.1alpha isn't out yet. So the code hasn't
been tested by many people yet.
A CRC function tries to produce the
Worth noting I've seen faults at avx2/512 for reads that aren't correctly
aligned, unless I've made a mistake somewhere along the way
On Fri, Jan 17, 2025 at 1:05 PM Paul Eggert wrote:
>
> On 2025-01-17 06:44, Jeffrey Walton wrote:
> > Change:
> >
> > const __m128i *data = buf;
> >
> > To this so the compiler cannot pick between MOVDQA and MOVDQU:
> >
> > const __m128i data = _mm_loadu_si128(buf);
>
> This doesn't look
On 2025-01-17 05:40, Simon Josefsson wrote:
Do you see any chance to merge your code into gnulib? If there is
any feature in your implementation that we don't have. And then use it
in xz?
Someone could merge xz's improvements into Gnulib because xz is 0BSD
which can donate to LGPLv3. Using G
Date: Fri, 17 Jan 2025 10:27:55 -0800
Subject: [PATCH 1/2] crc-x86_64: better fix for unaligned access
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Avoid undefined behavior in a way that doesn’t require
the input buffer to be aligned.
From a suggestion by
On 2025-01-17 06:44, Jeffrey Walton wrote:
Change:
const __m128i *data = buf;
To this so the compiler cannot pick between MOVDQA and MOVDQU:
const __m128i data = _mm_loadu_si128(buf);
This doesn't look right, as 'data' is used as a pointer later.
On Fri, Jan 17, 2025 at 3:40 AM Sam Russell wrote:
>
> We discussed this previously, we decided since AVX1 supports unaligned
> accesses we could not do an alignment check at the start of the function, but
> as you've discovered, this memcpy issue creates undefined behaviour.
I don't believe th
Lasse Collin writes:
> [1]
> https://github.com/tukaani-project/xz/blob/master/src/liblzma/check/crc_x86_clmul.h
Oh. Do you see any chance to merge your code into gnulib? If there is
any feature in your implementation that we don't have. And then use it
in xz? It seems a bit silly to have a
I'm fairly new on the list and haven't read earlier discussion about
the crc-x86_64 module, so I hope this doesn't come out wrong. I rewrote
the CRC CLMUL code[1] in XZ Utils six months ago, and I'm commenting
based on that experience.
On 2025-01-16 Paul Eggert wrote:
> T
please read "we could not do" as "we could avoid doing"
We discussed this previously, we decided since AVX1 supports unaligned
accesses we could not do an alignment check at the start of the function,
but as you've discovered, this memcpy issue creates undefined behaviour.
Most performant would probably be an alignment check at the start and then
manua
ificant performance issue in
Gnulib-using code, I hope Sam or somebody can come up with a
higher-performance fix.From 59146234323977ae8815a6b8aa8c5a81298b19bb Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Thu, 16 Jan 2025 22:58:55 -0800
Subject: [PATCH] crc-x86_64: fix unaligned access
MIME-
On Fri, Jan 17, 2025 at 12:07 AM Bruno Haible via Gnulib discussion
list wrote:
>
> Paul Eggert wrote:
> > > ../lib/crc-x86_64-pclmul.c:120:26: runtime error: load of misaligned
> > > address 0x5572a8d5f161 for type 'const __m128i *', which requires 16 byte
&g
Paul Eggert wrote:
> > ../lib/crc-x86_64-pclmul.c:120:26: runtime error: load of misaligned
> > address 0x5572a8d5f161 for type 'const __m128i *', which requires 16 byte
> > alignment
> > 0x5572a8d5f161: note: pointer points here
> > 2a 27 00 fc 75 47
On 2025-01-16 11:29, Bruno Haible via Gnulib discussion list wrote:
../lib/crc-x86_64-pclmul.c:120:26: runtime error: load of misaligned address
0x5572a8d5f161 for type 'const __m128i *', which requires 16 byte alignment
0x5572a8d5f161: note: pointer points here
2a 27 00 fc 75 47 2
Hi Sam,
Testing the current coreutils with the current gnulib, there is an
undefined behaviour in crc-x86_64-pclmul.c.
Found by building on Ubuntu 24.04, with clang 19,
CC="clang
-fsanitize=address,undefined,signed-integer-overflow,shift,integer-divide-by-zero
-fno-sanitize-recover=unde
In Coreutils, which uses --enable-silent-rules by default, I see:
$ make
GEN .version
GEN lib/alloca.h
GEN lib/arpa/inet.h
GEN lib/configmake.h
if test -n 'gcc'; then \
/usr/bin/mkdir -p 'lib/crc-tmp' \
&am
Bruno Haible via Gnulib discussion list writes:
> So, all in all, it seems that the use of __attribute__ and #pragmas
> is easier to maintain, in the long run, than compiler options.
I agree with your analysis here too, even though I expressed support for
the automake helper library approach in
Bruno Haible via Gnulib discussion list writes:
> Makefile.am:
> if GL_CRC_X86_64_PCLMUL
> -lib_SOURCES += crc-x86_64-pclmul.c
> +# We need a separate library, in order to compile crc-x86_64-pclmul.c with
> +# particular CFLAGS.
> +# (Recall that '#pragma GCC target (..
this boils down
to the question: Which of the two approaches generalizes better
to other compilers?
Looking at possible MSVC support: MSVC does not need specific
options (although /Oi can be used) [1], and just needs a different
#include [2]. What that simple change, crc-x86_64-pclmul.c compiles
f
Hi Bruno,
Bruno Haible via Gnulib discussion list writes:
> I picked approach c), and committed the two attached patches. Tested both
> without libtool (in a testdir) and with libtool (in some package that uses
> gnulib for some shared library).
Would __attribute__ ((target ("..."))) not work h
, and it failed today:
>
> clang -DHAVE_CONFIG_H -I. -I../../gllib -I.. -DGNULIB_STRICT_CHECKING=1
> -I/media/develdata/devel/inst-x86_64-64/include -Wall -g -O2 -MT
> crc-x86_64-pclmul.o -MD -MP -MF .deps/crc-x86_64-pclmul.Tpo -c -o
> crc-x86_64-pclmul.o ../../gllib/crc-x86_64-pcl
The gnulib CIs also exercise compilation with clang, and it failed today:
clang -DHAVE_CONFIG_H -I. -I../../gllib -I.. -DGNULIB_STRICT_CHECKING=1
-I/media/develdata/devel/inst-x86_64-64/include -Wall -g -O2 -MT
crc-x86_64-pclmul.o -MD -MP -MF .deps/crc-x86_64-pclmul.Tpo -c -o
crc-x86_64
da6b90f1fd041c013b9685ec4256e16d75b83f5d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?=
Date: Wed, 18 Dec 2024 12:53:02 +
Subject: [PATCH] crc-x86_64: fix build failure due to indentation
* modules/crc-x86_64: Remove indentation on lib_SOURCES,
as otherwise it's replaced
Bruno Haible writes:
> Simon Josefsson wrote:
>> Thanks - I have pushed this now.
>
> Two more nits, that I am fixing:
Looks good, thank you!
/Simon
signature.asc
Description: PGP signature
we should not reuse
the same column number for nested indentation levels.
2024-12-17 Bruno Haible
crc-x86_64: Tweaks.
* modules/crc-x86_64 (Include): Fix the file name.
* m4/crc-x86_64.m4 (gl_CRC_X86_64_PCLMUL): Improve indentation.
diff --git a/m4/crc-x86_64.m4 b/
> Thanks - I have pushed this now.
Thanks :)
> My point was that it would be better to use the same mechanism the .c
> implementation uses in the ./configure check, to make sure it tests for
> what will be used. That is, drop the CFLAGS changes and add:
I agree from a testability perspective, I
gt; discussed with Bruno earlier in the thread, CFLAGS set this for the entire
> build so we've done it with pragmas inside crc-x86_64-pclmul.c
My point was that it would be better to use the same mechanism the .c
implementation uses in the ./configure check, to make sure it tests for
what
comment saying where it is coming from, like:
fixed, code is original but copyright changed to just 2024
> Could you run 'indent' on this file? I think these should be '{ 0 }'
> and '...si128 ();' respectively.
done
> Shouldn't this be crc-x86_64-pclmul.h
ith that request unless you want to clean it up :)
I'm now ready to push when these nits are corrected:
> Subject: [PATCH] crc: Add PCLMUL implementation
>
> * lib/crc-x86_64-pclmul.c: Implement CRC32 with PCLMUL intrinsics
> * lib/crc-x86_64.h: Add header for CRC32 with PCLMUL
Thanks for the quick reply Bruno, updated patch attached
On Mon, 16 Dec 2024 at 17:25, Bruno Haible wrote:
> Sam Russell wrote:
> > crc-x86_64 is added as its own package with minimal bindings in crc.c to
> > support, flagged off by the GL_CRC_PCLMUL flag which is only set if the
Sam Russell wrote:
> crc-x86_64 is added as its own package with minimal bindings in crc.c to
> support, flagged off by the GL_CRC_PCLMUL flag which is only set if the
> crc-x86_64 package is selected.
Thanks. Some final nitpicking from my side:
In crc-x86_64-pclmul.c:
&g
crc-x86_64 is added as its own package with minimal bindings in crc.c to
support, flagged off by the GL_CRC_PCLMUL flag which is only set if the
crc-x86_64 package is selected.
Tested against gzip with the package included and omitted and gzip builds
fine in both case.
On Fri, 13 Dec 2024 at 20
>> > CRC32, so there will be more patches coming in future to match the ones
>> > I've submitted to coreutils. Are there any volunteers to be maintainer for
>> > these?
>> >
>> > On Fri, Dec 13, 2024, 17:52 Sam Russell wrote:
>> >>
>&g
#x27;ve submitted to coreutils. Are there any volunteers to be maintainer for
> these?
> >> >
> >> > On Fri, Dec 13, 2024, 17:52 Sam Russell
> wrote:
> >> >>
> >> >> > I'd prefer of the crc PCLMUL feature is "opt-in" from
uture to match the ones
> I've submitted to coreutils. Are there any volunteers to be maintainer for
> these?
> >
> > On Fri, Dec 13, 2024, 17:52 Sam Russell wrote:
> >>
> >> > I'd prefer of the crc PCLMUL feature is "opt-in" from a package
fficient algorithms for CRC32, so
> there will be more patches coming in future to match the ones I've submitted
> to coreutils. Are there any volunteers to be maintainer for these?
>
> On Fri, Dec 13, 2024, 17:52 Sam Russell wrote:
>>
>> > I'd pre
these?
On Fri, Dec 13, 2024, 17:52 Sam Russell wrote:
> > I'd prefer of the crc PCLMUL feature is "opt-in" from a package
> > maintainer point of view.
>
> fine by me, this just requires a change to the build script then? the
> binding in crc.c has #ifdefs ar
> I'd prefer of the crc PCLMUL feature is "opt-in" from a package
> maintainer point of view.
fine by me, this just requires a change to the build script then? the
binding in crc.c has #ifdefs around it already
> So this would be removed.
ok, so we just need crc pclmul
Thanks for continuing work on this! I'd like to focus on a meta issue
first. I'd prefer of the crc PCLMUL feature is "opt-in" from a package
maintainer point of view. Do you see any problem with that? What I'm
after is that people who import the 'crc' module
Simon,
Sam Russell wrote:
> Just bumping this, is this a change we would like to include? Are there any
> issues with the patch that you'd like me to fix?
This module is in your responsibility, Simon. What's your take on Sam's
current patch? [1]
Bruno
[1] https://lists.gnu.org/archive/html/bug-
removed based on Jeff's feedback
> that _mm_loadu_si128() accepts unaligned data. bench-crc, test-crc and gzip
> all happy.
>
> Am I correct in thinking that we are just waiting for Simon's input now?
>
> On Tue, 26 Nov 2024 at 22:53, Bruno Haible wrote:
>
&g
hoose in the end.
Final patch for now with alignment removed based on Jeff's feedback
that _mm_loadu_si128() accepts unaligned data. bench-crc, test-crc and gzip
all happy.
Am I correct in thinking that we are just waiting for Simon's input now?
On Tue, 26 Nov 2024 at 22:53, Bruno H
Sam Russell wrote:
> It makes sense to keep them in the same module though, I agree.
Thanks.
> I'd prefer to keep them as separate files if you're okay with it. I did a
> quick experiment and by wrapping each function in push_options and
> pop_options pragmas it was pretty easy to get it all work
de.
> I believe the way to zero a __m128i is using _mm_setzero_si128(). It
Perfect, it's outside the main loop so I'm not worried but this does look
more correct.
On Tue, 26 Nov 2024 at 22:47, Jeffrey Walton wrote:
> On Tue, Nov 26, 2024 at 4:27 PM Sam Russell
> wrote:
> >
On Tue, Nov 26, 2024 at 4:27 PM Sam Russell wrote:
>
> I've added an alignment check in lib/crc, it looks like the code works okay
> without it for me but an _m128 is supposed to be 128-bit aligned so I'm happy
> that I've added it.
The _m128i's are na
I've added an alignment check in lib/crc, it looks like the code works okay
without it for me but an _m128 is supposed to be 128-bit aligned so I'm
happy that I've added it.
The attached patch renames the module to crc-x86_64 while keeping the
source file crc-x86_64-pclmul.c,
though, I agree. Ultimately we can delay this decision
until we have the avx2/avx512 implementations, I'll rename the module
to crc-x86_64 in the meantime though.
Thanks for the updated patch. I'm fine with the 'crc-x64_64-pclmul' name.
Sam Russell wrote:
> > * Are the options -mpclmul -mavx understood by both gcc and clang?
> > Or does clang use different options for the same thing?
>
> As per [1] it looks to be the c
> * I would suggest to rename the main source file from crc-pclmul.c to
> crc-x86_64.c.
> Rationale: So that immediately clear that the code is specific to the
> x86_64 CPUs. Not everyone is an assembly language hacker, and even
some
> assembly language hackers (like
Hi Sam,
Thanks for working on this!
Sam Russell wrote:
> 85% time reduction on AMD Ryzen 5 5600:
>
> $ ./gltests/bench-crc 100
> real 1.740296
> user 1.740
> sys0.000
>
> $ ../bench-crc-pclmul 100
> real 0.248324
> user 0.248
> sys0
85% time reduction on AMD Ryzen 5 5600:
$ ./gltests/bench-crc 100
real 1.740296
user 1.740
sys0.000
$ ../bench-crc-pclmul 100
real 0.248324
user 0.248
sys0.000
This translates to a 13% time reduction for gzip:
$ time ./gzip_sliceby8 -k -d -c large_file.gz > /dev/n
Pádraig Brady wrote:
> Latest gnulib is failing coreutils CI because of a mismatch in mkdir && cd
> due to prefixes.
> In the generated Makefile I'm seeing: $(MKDIR_P) lib/crc-tmp
> I.e. MKDIR_P has been processed to add in the lib prefix.
> However the subsequent `
On 31/10/2024 13:51, Bruno Haible via Gnulib discussion list wrote:
Simon Josefsson wrote:
I merged this now, thank you!
And here's the cross-compilation handling that I promised to add.
2024-10-31 Bruno Haible
crc: Don't attempt to run a compiled C program when cross
7;s the cross-compilation handling that I promised to add.
> >
> >
> > 2024-10-31 Bruno Haible
> >
> > crc: Don't attempt to run a compiled C program when
> cross-compiling.
>
> Here's an improvement:
>
>
> 2024-10-31 Bruno Haible
>
ters.
> >
> > What do you think?
>
> Ouch. Ideally maybe uint8_t should have been used, but both uint8_t and
> void* is an API change compared to current char*.
While the crc _implementation_ uses uint8_t*, from the caller's perspective:
the caller passes the bounds of s
Bruno Haible via Gnulib discussion list writes:
> Hi Simon,
>
> Compiling a testdir of module 'crc' with "gcc -Wall", I see these warnings:
...
> I think that the proper fix is to change the prototypes of the 4 functions
> in crc.h from 'const char *buf
I wrote:
> And here's the cross-compilation handling that I promised to add.
>
>
> 2024-10-31 Bruno Haible
>
> crc: Don't attempt to run a compiled C program when cross-compiling.
Here's an improvement:
2024-10-31 Bruno Haible
crc: Suppor
tils.
I'll send a patch tomorrow for that.
> I'd be especially reluctant to add the existing cksum crc variant to gnulib,
> since I don't see anything but cksum itself using that.
I need to research a little further, but bzip2 uses the same implementation as
cksum afaik
Y
On Thu, Oct 31, 2024 at 5:15 PM Simon Josefsson via Gnulib discussion
list wrote:
>
> Pádraig Brady writes:
>
> > I'd be reluctant to complicate the gnulib implementations with variants
> > unless needed.
> > I'd be especially reluctant to add the existing c
On 31/10/2024 21:15, Simon Josefsson wrote:
Pádraig Brady writes:
I'd be reluctant to complicate the gnulib implementations with variants unless
needed.
I'd be especially reluctant to add the existing cksum crc variant to gnulib,
since I don't see anything but cksum itself u
Hi Simon,
Compiling a testdir of module 'crc' with "gcc -Wall", I see these warnings:
gcc -ftrapv -DHAVE_CONFIG_H -I. -I../../gltests -I..
-DGNULIB_STRICT_CHECKING=1 -DIN_GNULIB_TESTS=1 -I. -I../../gltests -I..
-I../../gltests/.. -I../gllib -I../../gltests/../gllib
-I/m
Pádraig Brady writes:
> I'd be reluctant to complicate the gnulib implementations with variants
> unless needed.
> I'd be especially reluctant to add the existing cksum crc variant to gnulib,
> since I don't see anything but cksum itself using that.
Is there some
kages use this variable with a different convention: value 1 or 0,
not yes or no. So, it's adequate to namespace it:
2024-10-31 Bruno Haible
crc: Avoid potential conflict with other configure.ac files.
Suggested by Simon Josefsson.
* modules/crc (configure.ac, Makefi
especially reluctant to add the existing cksum crc variant to
gnulib,
> since I don't see anything but cksum itself using that.
I need to research a little further, but bzip2 uses the same implementation
as cksum afaik
On Thu, 31 Oct 2024 at 18:33, Pádraig Brady wrote:
> On 31/10/2024 16
y has the -a option to select between algorithms.
`cksum -a crc` currrently select cksums POSIX crc
(which also munges in the data length to the crc calculation).
`cksum -a crc32` could be added I suppose to select the current implementation
in gnulib
(and not munge in the length). I can add that e
ing needs to happen, we
> can just map different offsets (so instead of data[0>, data[1] we can count
> the other direction, or reverse the order of the lookup tables at compile
> time)
> Reverse CRC: easy to have as a run-time parameter.
>
> The pclmul is a little trickier,
different offsets (so instead of data[0>, data[1] we can count
the other direction, or reverse the order of the lookup tables at compile
time)
Reverse CRC: easy to have as a run-time parameter.
The pclmul is a little trickier, the intel paper covers the reversed
polynomial case though. We
On 31/10/2024 12:18, Simon Josefsson via Gnulib discussion list wrote:
I merged this now, thank you!
FYI, I looked at using this from coreutils cksum,
but unfortunately that uses a different CRC-32 variant.
For my reference...
coreutils cksum parameters:
Polynomial
Bruno Haible via Gnulib discussion list writes:
> Simon Josefsson wrote:
>> I merged this now, thank you!
>
> And here's the cross-compilation handling that I promised to add.
Both this and the other one looks fine, thank you!
> configure.ac:
> AC_REQUIRE([gl_CRC_SLICE_BY_8])
> +CROSS_COMPILI
Bruno Haible via Gnulib discussion list writes:
> Sam Russell wrote:
>> Patch attached to mark functions in crc.c and crc-generate-tables.c as
>> static, GZIP now builds
>
> The patch looks good to me. Simon?
Pushed, thank you!
/Simon
signature.asc
Description: PGP signature
s://www.gnu.org/software/gnulib/manual/html_node/Modified-build-rules.html
> > There's another issue with the way fopen/fprintf work and I can't
> > figure out how to fix it:
>
> Sounds like you need to link crc-generate-table with the gnulib
> replacement library.
Th
t; an error when using += with it.
I think you should add 'MAINTAINERCLEANFILES =' to gzip instead.
Otherwise I suspect 'make distcheck' will not work (please test that in
gzip after final changes).
> There's another issue with the way fopen/fprintf work and I can&#x
Sam Russell wrote:
> Patch attached to mark functions in crc.c and crc-generate-tables.c as
> static, GZIP now builds
The patch looks good to me. Simon?
> $ time ./gzip_old -d -k large_file.gz -c > /dev/null
>
> real0m0.491s
> user0m0.491s
> sys 0m0.000s
>
> I believe this should be fixed through the second patch of mine.
Yes, thanks
> I think you should add 'MAINTAINERCLEANFILES =' to gzip instead.
Adding to lib/Makefile.am on line 24 fixes this
Patch attached to mark functions in crc.c and crc-generate-tables.c as
static,
Sam Russell wrote:
> /usr/bin/ld: /tmp/cc06ZLWr.o: in function `main':
> /home/sam/code/gzip/lib/./crc-generate-table.c:139: undefined reference to
> `rpl_fopen'
> /usr/bin/ld: /tmp/cc06ZLWr.o: in function `print_copyright_notice':
> /home/sam/code/gzip/lib/./crc-g
Simon Josefsson wrote:
> I merged this now, thank you!
And here's the cross-compilation handling that I promised to add.
2024-10-31 Bruno Haible
crc: Don't attempt to run a compiled C program when cross-compiling.
* lib/crc-generate-table.c: Don'
oing a "chmod a-w" of the file.
2024-10-31 Bruno Haible
crc: Tweak generator.
* lib/crc-generate-table.c (print_header): Don't emit a blank line at
the end.
(print_copyright_notice): Prepend a "DO NOT EDIT" line.
(main): Fail
test suite too, to add some assurance we didn't mess up
anything.
A next step would be to evaluate if the assembler CRC instructions help
further on some platforms...
/Simon
From cc3fb685ac370a368cb84480117be13d109eebf0 Mon Sep 17 00:00:00 2001
From: Simon Josefsson
Date: Thu, 31 Oct 2024
other issue with the way
fopen/fprintf work and I can't figure out how to fix it:
/usr/bin/ld: /tmp/cc06ZLWr.o: in function `main':
/home/sam/code/gzip/lib/./crc-generate-table.c:139: undefined reference to
`rpl_fopen'
/usr/bin/ld: /tmp/cc06ZLWr.o: in function `print_copyright_not
Sam Russell wrote:
> Bruno, can you please confirm whether you're happy with my implementation
> of the AC_ARG_ENABLE invocation?
Yes, it looks correct.
The serial number 3 is correct too, since there was a crc.m4 serial 2
in the past.
Bruno
toconf (inspired heavily by m4/debug.m4), can confirm
>> the behaviour works as follows:
>>
>> # build with slice-by-8 (default enabled)
>> ./configure
>> make
>> gltests/bench-crc 100
>> real 1.714850
>> user 1.715
>> sys0.000
>
okt. 2024 kl. 23:04 skrev Sam Russell :
>
>
> I had a play with autoconf (inspired heavily by m4/debug.m4), can confirm
> the behaviour works as follows:
>
> # build with slice-by-8 (default enabled)
> ./configure
> make
> gltests/bench-crc 100
> real 1.714850
(inspired heavily by m4/debug.m4), can confirm the behaviour works as follows:# build with slice-by-8 (default enabled)./configuremakegltests/bench-crc 100real 1.714850user 1.715sys 0.000# build without slice-by-8./configure --disable-crc-slice-by-8makegltests/bench-crc 100real
I had a play with autoconf (inspired heavily by m4/debug.m4), can confirm
the behaviour works as follows:
# build with slice-by-8 (default enabled)
./configure
make
gltests/bench-crc 100
real 1.714850
user 1.715
sys0.000
# build without slice-by-8
./configure --disable-crc-slice-by-8
Simon Josefsson wrote:
> I am not certain about the method to enable/disable the optimization,
> is the configure.ac variable the best method we have?
An AC_ARG_ENABLE invocation can be added. Since Sam said that he's not familiar
with Autoconf, it would be your turn to add it...
Bruno
Looks good to me too - although I am not certain about the method to
enable/disable the optimization, is the configure.ac variable the best method
we have?
/Simon
> 27 okt. 2024 kl. 14:20 skrev Bruno Haible :
>
> Looks good to me now.
>
> Simon?
>
>
>
Looks good to me now.
Simon?
he operator. [1]
> > > This affects lib/crc.c lines 73..80, 102..103.
> >
> > Done.
>
> I meant this expression:
>
> crc = crc32_sliceby8_table[0][(local_buf >> 56) & 0xFF] ^
> crc32_sliceby8_table[1][(local_buf >> 48) & 0xFF] ^
>
Sam Russell wrote:
> > Also, in GNU coding style, when line breaking is needed within
> expressions,
> > we do the line break before the operator, not after the operator. [1]
> > This affects lib/crc.c lines 73..80, 102..103.
>
> Done.
I meant this expression:
c
at 12:58, Bruno Haible wrote:
> Sam Russell wrote:
> > I used Simon's indent trick on both files
>
> Unfortunately, 'indent' introduced tabs. In Gnulib, we indent with spaces,
> not tabs (except in Makefiles and ChangeLog). Can you please untabify:
> $ expand &
Addendum to:
> Unfortunately, 'indent' introduced tabs. In Gnulib, we indent with spaces,
> not tabs (except in Makefiles and ChangeLog).
We have some documentation about this:
https://www.gnu.org/software/gnulib/manual/html_node/Indent-with-spaces-not-TABs.html
Bruno
Sam Russell wrote:
> I used Simon's indent trick on both files
Unfortunately, 'indent' introduced tabs. In Gnulib, we indent with spaces,
not tabs (except in Makefiles and ChangeLog). Can you please untabify:
$ expand < lib/crc.c > lib/crc.cx && mv lib/crc
ommended earlier.
> Regarding GNU coding style, there are still tweaks needed in
> crc-generate-table.c lines 54, 56, 83, 87, 100..114, 120..123,
> crc.c line 92.
I used Simon's indent trick on both files, if there's anything that I
missed here then posting a diff would make l
Thanks Sam, I think we are closing in on a good patch, and Bruno's
feedback seems good. Just one small piece of hint:
Bruno Haible via Gnulib discussion list writes:
> 3) Use GNU coding style:
You can fix most of these by simply running 'indent' on your *.h and *.c
files, and then manually rev
> > 2) Generate the tables at build time.
> >Step 1: Move the tables into a file lib/crc-sliceby8.h
>
> done
>
> > Step 2: Extend your program lib/crc-generate-table.c
>
> done
>
> >Step 3: Write a Makefile.am rule, in modules/crc, that runs the
1 - 100 of 134 matches
Mail list logo
