code review 6259043: crypto: amd64 assembly for hashes

crypto: amd64 assembly for hashes

Tuning the Go versions was a good source of things to fix
in the compiler, but ultimately the assembly is going to win.
Can't beat 'em, so join 'em.

Only adding assembly for SHA1, but set up for assembly
from others, if we can find some with acceptable licensing.

benchmark                  old MB/s     new MB/s  speedup
sha1.BenchmarkHash8K          87.31       369.56    4.23x

[rsc, 2012-05-25 04:15:58]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2012-05-25 04:16:57]

+agl

[r, 2012-05-25 04:39:13]

worried about licensing.

http://codereview.appspot.com/6259043/diff/7001/src/pkg/crypto/gen.sh
File src/pkg/crypto/gen.sh (right):

http://codereview.appspot.com/6259043/diff/7001/src/pkg/crypto/gen.sh#newcode6
src/pkg/crypto/gen.sh:6: OPENSSL=${OPENSSL:-$HOME/Downloads/openssl-1.0.1c}
this doesn't feel very general.

http://codereview.appspot.com/6259043/diff/7001/src/pkg/crypto/gen.sh#newcode16
src/pkg/crypto/gen.sh:16: s/BSWAP	/BSWAPL	/g
a shell comment wouldn't feel out of place here

http://codereview.appspot.com/6259043/diff/7001/src/pkg/crypto/md5/block_amd64.s
File src/pkg/crypto/md5/block_amd64.s (right):

http://codereview.appspot.com/6259043/diff/7001/src/pkg/crypto/md5/block_amd6...
src/pkg/crypto/md5/block_amd64.s:4: // then converted to Go assembly syntax.
do we need licensing stuff here for that?

http://codereview.appspot.com/6259043/diff/7001/src/pkg/crypto/md5/block_amd6...
src/pkg/crypto/md5/block_amd64.s:31: 
dddddddd?

http://codereview.appspot.com/6259043/diff/7001/src/pkg/crypto/sha1/sha1_test.go
File src/pkg/crypto/sha1/sha1_test.go (right):

http://codereview.appspot.com/6259043/diff/7001/src/pkg/crypto/sha1/sha1_test...
src/pkg/crypto/sha1/sha1_test.go:84: var buf = makeBuf()
you could do flag.Lookup("test.bench") to see whether it's worth doing this.
might be nice to have a cleaner way to make the decision.

[rsc, 2012-05-25 04:56:39]

PTAL

Added more comments, explicit copy of license.
Not worried about the blank lines in the generated .s files,
since they're generated.
The 8k make in the benchmarks seems okay to do
unconditionally too.

[fw, 2012-05-25 05:49:24]

http://codereview.appspot.com/6259043/diff/24/src/pkg/crypto/sha512/block_amd...
File src/pkg/crypto/sha512/block_amd64.s (right):

http://codereview.appspot.com/6259043/diff/24/src/pkg/crypto/sha512/block_amd...
src/pkg/crypto/sha512/block_amd64.s:99: *    "This product includes software
written by Tim Hudson (tjh@cryptsoft.com)"
Please keep the Go library under a GPL-compatible license.  The above isn't.

[rsc, 2012-05-25 05:52:31]

http://codereview.appspot.com/6259043/diff/24/src/pkg/crypto/sha512/block_amd...
File src/pkg/crypto/sha512/block_amd64.s (right):

http://codereview.appspot.com/6259043/diff/24/src/pkg/crypto/sha512/block_amd...
src/pkg/crypto/sha512/block_amd64.s:99: *    "This product includes software
written by Tim Hudson (tjh@cryptsoft.com)"
On 2012/05/25 05:49:24, fw wrote:
> Please keep the Go library under a GPL-compatible license.  The above isn't.

Since we didn't include any windows code from the apps directory, this clause
does not apply.

[fw, 2012-05-25 06:19:55]

>
http://codereview.appspot.com/6259043/diff/24/src/pkg/crypto/sha512/block_amd...
> File src/pkg/crypto/sha512/block_amd64.s (right):
>
>
http://codereview.appspot.com/6259043/diff/24/src/pkg/crypto/sha512/block_amd...
> src/pkg/crypto/sha512/block_amd64.s:99: *    "This product includes
> software written by Tim Hudson (tjh@cryptsoft.com)"
> On 2012/05/25 05:49:24, fw wrote:
>> Please keep the Go library under a GPL-compatible license.  The above
> isn't.
>
> Since we didn't include any windows code from the apps directory, this
> clause does not apply.

Sorry, I didn't select enough lines in the tool.  This is equally
problematic:

* 3. All advertising materials mentioning features or use of this
+ * software must display the following acknowledgment:
+ * "This product includes software developed by the OpenSSL Project
+ * for use in the OpenSSL Toolkit. (http://www.openssl.org/)"

+ * 6. Redistributions of any form whatsoever must retain the following
+ * acknowledgment:
+ * "This product includes software developed by the OpenSSL Project
+ * for use in the OpenSSL Toolkit (http://www.openssl.org/)"

+ * 3. All advertising materials mentioning features or use of this software
+ * must display the following acknowledgement:
+ * "This product includes cryptographic software written by
+ * Eric Young (eay@cryptsoft.com)"
+ * The word 'cryptographic' can be left out if the rouines from the library
+ * being used are not cryptographic related :-).

At the very least, you should include these required notices in the
top-level license file, so that Go programmers have a better chance of
compliance with these requirements.

[rsc, 2012-05-25 06:22:05]

*** Abandoned ***