Skip to content

md5: add x86_64 and AArch64 asm backends - #917

Open
sjthomason wants to merge 1 commit into
RustCrypto:masterfrom
sjthomason:md5-asm
Open

sjthomason wants to merge 1 commit into
RustCrypto:masterfrom
sjthomason:md5-asm

Conversation

@sjthomason

Copy link
Copy Markdown

Port the implementations to Rust inline assembly based on: https://github.com/animetosho/md5-optimisation

The original work was placed in the public domain: animetosho/md5-optimisation#4

@oech3

oech3 commented Sep 21, 2026 •

Copy link
Copy Markdown

The original work was placed in the public domain: animetosho/md5-optimisation#4

For maintainers, the original work changed license to Apache2.0/MIT .

Port the implementations to Rust inline assembly based on:
https://github.com/animetosho/md5-optimisation
@tarcieri

tarcieri commented Sep 22, 2026 •

Copy link
Copy Markdown
Member

I think it'd be helpful if this were split up into separate PRs for AArch64 vs x86(_64). Even #447 went by the wayside because these kinds of ASM backends are hard-to-review, even when adapted from seemingly mature sources of ASM. Though while this ASM seems to have a decent rationale, I'm not finding a whole lot in the way of a test suite.

Proptests which compare the Rust reference implementation with the ASM implementation would be nice. Also from what I can tell CI isn't even testing aarch64 whatsoever, just ensuring it compiles.

It would also be helpful if these changes were justified with benchmarks.

Edit: I looked for formally verified ASM for MD5 and wasn't able to find any

@oech3

oech3 commented Sep 23, 2026

Copy link
Copy Markdown

I looked for formally verified ASM for MD5 and wasn't able to find any

Author of https://github.com/animetosho/md5-optimisation said most of them are included to OpenSSL. It might be able to reference OpenSSL's md5.

@oech3

oech3 commented Sep 25, 2026

Copy link
Copy Markdown

CI isn't even testing aarch64 whatsoever, just ensuring it compiles.

Does this help?
https://github.com/oech3/rustcrypto-hashes/commit/823b8757ecb94d7e0033eb6234879911b7b24430

@tarcieri

Copy link
Copy Markdown
Member

That probably works, though you can look at polyval for how we've otherwise set up the matrix: https://github.com/RustCrypto/universal-hashes/blob/7232d5e23fd2d24ae3ce7b8944e6840d7655db6e/.github/workflows/polyval.yml#L36-L80

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants