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

Move avo assembly generator to separate asm module #149

Merged
merged 5 commits into from
Jan 21, 2025

Conversation

mmcloughlin
Copy link
Contributor

Move avo code-generator to an asm sub-module. This avoids the problem of the sha1cd module itself depending on avo.

Fixes #148

@pjbgf
Copy link
Owner

pjbgf commented Jan 18, 2025

@mmcloughlin thank you for proposing this PR, that's much appreciated. Please amend the make verify target, so that it points to the new asm/asm.go which should fix the build.

@mmcloughlin
Copy link
Contributor Author

@mmcloughlin thank you for proposing this PR, that's much appreciated. Please amend the make verify target, so that it points to the new asm/asm.go which should fix the build.

Thanks @pjbgf. I have pushed a new iteration with changes:

  • Moved the ubc code generator to its own asm sub-module.
  • Renamed the ubc/doc.go to ubc/ubc.go and added the go:generate line there too
  • Updated make generate to just call go generate -x ./...
  • Removed sed lines that add // +build constraints. I think go:build is sufficiently established that there's no need to keep the old form, unless you want to?

The only remaining thing I'd say is that the shared package doesn't make much sense anymore in this refactor at the moment. To avoid trying to share an internal package from the main module I just copied in the few constants needed by the assembly generator. So perhaps now internal/const.go should just be moved to the top-level package (and made private)?

Comment on lines +12 to +15
RoundConst0 = 0x5A827999
RoundConst1 = 0x6ED9EBA1
RoundConst2 = 0x8F1BBCDC
RoundConst3 = 0xCA62C1D6
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note they can't be named K0, K1, ... anymore since these are registers in avo (AVX-512 mask registers).

@pjbgf
Copy link
Owner

pjbgf commented Jan 20, 2025

So perhaps now internal/const.go should just be moved to the top-level package (and made private)?

That makes sense. Overall, the changes LGTM. It is up to you if you prefer making the additional change for the shared package here, or on a follow-up PR.

Copy link
Owner

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmcloughlin I will be merging this as is, thank you once again for proposing this change. The shared package change can be done at a later time.

@pjbgf pjbgf marked this pull request as ready for review January 21, 2025 10:31
@pjbgf pjbgf merged commit 05a0266 into pjbgf:main Jan 21, 2025
10 checks passed
@mmcloughlin
Copy link
Contributor Author

Great! Happy to help.

Yes the shared package wasn't a blocking issue.

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.

Unnecessary avo dependency
2 participants