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

Add a zkASM architecture #94

Merged

Conversation

nagisa
Copy link
Contributor

@nagisa nagisa commented Sep 18, 2023

This is a very initial take on this if only to be able to specify zkasm-unknown-unknown target in tooling that uses target-lexicon. Some of the details are not exactly 100% correct in the current state of the machine (e.g. pointer width is 256 bits), but this will potentially change soon…

src/targets.rs Outdated Show resolved Hide resolved
@nagisa nagisa force-pushed the nagisa/adds-zkasm-architecture branch from 38469bf to 8fd4f82 Compare September 18, 2023 12:31
@nagisa
Copy link
Contributor Author

nagisa commented Oct 5, 2023

@sunfishcode would it be possible to get a review/merge here?

@sunfishcode
Copy link
Member

zkasm doesn't appear to be a Rust or LLVM target name. We can potentially add it, but since I'm not familiar with zkasm at all: how confident are you that "zkasm" will remain the name of this architecture, and that no other architecture will call itself "zkasm", for the forseeable future?

@nagisa
Copy link
Contributor Author

nagisa commented Oct 6, 2023

zkasm doesn't appear to be a Rust or LLVM target name.

This is expected – we’re just now are experimenting with implementing a backend for this architecture in cranelift. Even then it might very well end up as an external backend, than something that's upstream. For the time being we’ve hijacked sparc to refer to our target, but that's quite inconvenient too.


We can potentially add it, but since I'm not familiar with zkasm at all: how confident are you that "zkasm" will remain the name of this architecture, and that no other architecture will call itself "zkasm", for the forseeable future?

Unfortunately I have little I can guarantee about either of these questions. The zkASM project is driven by the Polygon team (which I have a contact with but am not a part of,) and since zkASM is not a trademark to the best of my knowledge I or Polygon have little control over what other groups come up with and how they name their projects. That said, a quick search on a search engine for this name will bring Polygon’s project up in the first page of the search results, so that’s as good as reservation can get for the time being?

For what it is worth, I’m perfectly happy with this target not being set in stone in perpetuity. I anticipate the benefit of doing so to be very low and I would imagine it wouldn’t be too onerous to repurpose or remove the architecture string from target-lexicon if a need to do so arises.

@sunfishcode
Copy link
Member

Yeah, target-lexicon is kind of in a somewhat awkward position, being used by cranelift which doesn't need to be limited to stable targets, but also having become more broadly used by libraries that want a stable API.

Would it make sense to add zkarch under a feature flag, like #[cfg(feature = "zkarch")], which cranelift could enable, and which could be documented like this?

[features]
...
# Enable unstable zkarch architecture support.
zkarch = []

@nagisa
Copy link
Contributor Author

nagisa commented Oct 11, 2023

Yeah, I think we can do that no problem. I’ll make the change tomorrow.

@nagisa nagisa force-pushed the nagisa/adds-zkasm-architecture branch from 8fd4f82 to 638180d Compare October 12, 2023 08:10
This is a very initial take on this if only to be able to specify
`zkasm-unknown-unknown` target in tooling that uses `target-lexicon`.
@nagisa nagisa force-pushed the nagisa/adds-zkasm-architecture branch from 638180d to a6d8fed Compare October 12, 2023 08:10
@nagisa
Copy link
Contributor Author

nagisa commented Oct 12, 2023

Done. I ended up picking arch_zkasm here for the feature name, with a thought that if there were ever further future extensions of this sort, it would make a lot of sense to have them behind arch_*, os_* or similar features as well.

@sunfishcode
Copy link
Member

Looks good, thanks!

@sunfishcode sunfishcode merged commit 1f9d02e into bytecodealliance:main Oct 19, 2023
12 checks passed
@sunfishcode
Copy link
Member

This is now released in target-lexicon 0.12.12.

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