Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/crypto/sha3: re-introduce s390x assembly #64897

Open
FiloSottile opened this issue Dec 29, 2023 · 4 comments
Open

x/crypto/sha3: re-introduce s390x assembly #64897

FiloSottile opened this issue Dec 29, 2023 · 4 comments
Assignees
Labels
arch-s390x Issues solely affecting the s390x architecture. NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone

Comments

@FiloSottile
Copy link
Contributor

The current s390x assembly hooks into the package in such a way that it disables devirtualization, and by extension inlining and successful escape analysis for every architecture.

Specifically, current functions look like

func New256() hash.Hash {
	if h := new256Asm(); h != nil {
		return h
	}
	return &state{rate: 136, outputLen: 32, dsbyte: 0x06}
}

making the return type conditional, which doesn't work with devirtualization.

https://go.dev/cl/544816 disables the assembly (without removing it, to make re-enabling it easier and to preserve the git history), so that https://go.dev/cl/544817 can make the necessary changes so that the sha3 APIs can be used with zero allocations. (See the new TestAllocations.)

We plan to submit CL 544816 on January 8th, after the next x/crypto release is tagged (meaning the change will show up in @latest at the beginning of February). @golang/s390x maintainers are welcome to reintroduce it afterwards, or to chain a CL after CL 544817 to be submitted at the same time.

@FiloSottile FiloSottile added NeedsFix The path to resolution is known, but the work has not been done. arch-s390x Issues solely affecting the s390x architecture. labels Dec 29, 2023
@gopherbot gopherbot added this to the Unreleased milestone Dec 29, 2023
@mauri870
Copy link
Member

mauri870 commented Jan 6, 2024

I'll chain in a CL enabling the s390x assembly, thanks.

Edit CL 554435

@mauri870 mauri870 self-assigned this Jan 6, 2024
@gopherbot
Copy link

Change https://go.dev/cl/554435 mentions this issue: sha3: reenable s390x assembly

@mauri870
Copy link
Member

mauri870 commented Feb 1, 2024

Any plans to submit said CLs @FiloSottile?

@FiloSottile
Copy link
Contributor Author

They are waiting for review, ping @rolandshoemaker.

(They also somehow make Apple Silicon performance worse, and I didn't have time to look into why yet, but it's not necessarily a blocker.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-s390x Issues solely affecting the s390x architecture. NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Development

No branches or pull requests

4 participants