Re: [PATCH] block-sha1: remove use of assembly

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Mar 8, 2022, 02:20 UTC
Message-ID: <Yia9dQ32uYCCpIsm@camp.crustytoothpaste.net>
In-Reply-To: <YiaWGrBNuk1+j89z@nand.local>

On 2022-03-07 at 23:32:42, Taylor Blau wrote:

Show 15 quoted lines

> On Mon, Mar 07, 2022 at 11:25:52PM +0000, brian m. carlson wrote:
> > In the block SHA-1 code, we have special assembly code for i386 and
> > amd64 to perform rotations with assembly.  This is supposed to help pick
> > the correct rotation operation depending on which rotation is smaller,
> > which can help some systems perform slightly better, since any circular
> > rotation can be specified as either a rotate left or a rotate right.
> > However, this isn't needed, so we should remove it.
> 
> At -O1 or higher (at least on GCC) this optimization is indeed
> performed. Here's a Godbolt example that shows this:
> 
>     https://godbolt.org/z/9zMP93hr1
> 
> so I agree that this code isn't helping us at all. And in the
> meantime...

Thanks for providing a link. I also did a similar test there with slightly different code (and unfortunately closed the window before saving the link) but it demonstrated the same thing: that the compiler can optimize this case adequately. My (substantially) past experience with testing GCC in this case has shown the same thing.

Show 12 quoted lines

> > The downside of using this code, however, is that it uses a GCC
> > extension, which makes the compiler complain when using -pedantic unless
> > it's prefixed with __extension__.  We could fix that, but since it's
> > not needed, let's just remove it.  We haven't noticed this because
> > almost everyone uses the SHA1DC code instead, but it still shows up for
> > some people.
> 
> ...it makes it impossible to compile git if you have
> `BLK_SHA1=YesPlease` and `DEVELOPER=1` in your environment. So I am
> happy to see this go.
> 
> On another note: missing Signed-off-by?

I'll send an otherwise unchanged v2 with that in a second.

-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA