On 17/12/2024 07:55, Simon Josefsson via Gnulib discussion list wrote:
Thanks - I have pushed this now.
Just testing this in coreutils (for use by `cksum -a crc32b`),
and it seems to be working, but only after I fix a build failure with the
attached.
Thanks for the work on this.
PádraigFrom
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
Simon Josefsson wrote:
> Thanks - I have pushed this now.
Two more nits, that I am fixing:
- The include file in the module description was incorrect.
- The indentation in the .m4 file. There is no formal standard of what
a "correct" indentation in a .m4 file. But at least we should not re
> 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
Thanks - I have pushed this now.
Sam Russell writes:
>> I'm unsure about those CFLAGS. Aren't these CFLAGS required when
>> building the code too? If not, why can't the same pattern to detect
>> PCLMUL during ./configure which is done during run-time?
>
> discussed with Bruno earlier in the th
> End sentences with '.'
done
> Same here.
done
> That filename comment is now wrong.
fixed
> Is that true? If this new original code, shouldn't it say 2024? If
> this isn't new original code (does it come from coreutils), it make
> sense to add a comment saying where it is coming from, lik
Sam Russell writes:
> Thanks for the quick reply Bruno, updated patch attached
I really like what you did here! It is clean and easy to review,
separated from the core crc.c code. In retrospect the slice-by-8
variant should have followed this pattern instead, but I won't pester
you with that r
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
> > crc-x86_64
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:
> +#include
> +
> +#includ
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:4
That's not the way it usually works.
Typically, the author and/or person most familiar with the code is
listed as the Maintainer.
If you don't want to list yourself and no one else steps up, just mark
it as "all".
On Fri, Dec 13, 2024 at 10:52 AM Sam Russell wrote:
>
> Jim, would you be willing t
Sounds good, thanks for explaining. I'll get going on the autoconf stuff
then.
On Fri, Dec 13, 2024, 20:03 Jim Meyering wrote:
> That's not the way it usually works.
> Typically, the author and/or person most familiar with the code is
> listed as the Maintainer.
> If you don't want to list yours
Jim, would you be willing to be the maintainer for this in gnulib? If so
then I can fix the autoconf this week and get it ready to go
On Fri, Dec 13, 2024, 19:29 Jim Meyering wrote:
> As soon as this is added to gnulib, I'll be happy to prepare for a new
> gzip release that includes it.
>
> On F
As soon as this is added to gnulib, I'll be happy to prepare for a new
gzip release that includes it.
On Fri, Dec 13, 2024 at 9:45 AM Sam Russell wrote:
>
> I should also add that I'm planning long term to try and get the major open
> source tools (e.g. gzip) using the most efficient algorithms
I should also add that I'm planning long term to try and get the major open
source tools (e.g. gzip) using the most efficient 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
> 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 to be explicitly flagged o
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 now don't automatically
get the CRC
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-
Hi guys,
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?
Cheers
Sam
On Tue, Nov 26, 2024, 23:01 Sam Russell wrote:
> > It's your choice: 3 compilation units for x86_64, or 1 compilation unit
> > for x86_64, or no ext
> It's your choice: 3 compilation units for x86_64, or 1 compilation unit
> for x86_64, or no extra compilation unit (all code contained in .h files)
—
> as you prefer. Fine with me either way.
Let's cross that bridge when we get to it :) I'm fairly relaxed which one
we choose in the end.
Final p
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
> Thue use of _mm_loadu_si128 provides for unaligned byte arrays (that's
> the 'u' in the 'loadu'), so you will be Ok there, too.
Thanks Jeff, I wasn't going to push this with a "works for me" without
knowing why. I'll remove the alignment code.
> I believe the way to zero a __m128i is using _mm_
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 naturally aligned. They will be ok:
+
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, as well as the alignm
> Cool. But it even gets better: one can use these target options on a per-
> function basis, via __attribute__. See
>
https://gcc.gnu.org/onlinedocs/gcc-14.2.0/gcc/x86-Function-Attributes.html#index-target_0028_0022avx_0022_0029-function-attribute_002c-x86
>
https://gcc.gnu.org/onlinedocs/gcc-14
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 case
Thanks for having checked it.
> * 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 me) don't know ab
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.000
>
> This translates to a 13% time
28 matches
Mail list logo
