bytecodealliance / target-lexicon

Target "triple" support
Apache License 2.0
48 stars 42 forks source link

Add a zkASM architecture #94

Closed nagisa closed 1 year ago

nagisa commented 1 year ago

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…

nagisa commented 1 year ago

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

sunfishcode commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

nagisa commented 1 year ago

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 commented 1 year ago

Looks good, thanks!

sunfishcode commented 1 year ago

This is now released in target-lexicon 0.12.12.