Remove RISC-V assembler #1852
Remove RISC-V assembler #1852
Conversation
| /// A single 32-bit data value. | ||
| pub enum SingleDataValue { | ||
| /// A literal value. | ||
| Value(u32), | ||
| /// The value of a pointer to a text label. Since there may be not a | ||
| /// 1-to-1 correspondence between nativa ISAs and Powdr ASM instructions, | ||
| /// this is passed unresolved to the code generator. | ||
| LabelReference(String), | ||
| /// Currently not supported. | ||
| Offset(String, String), | ||
| } | ||
|
|
||
| pub fn quote(s: &str) -> String { | ||
| // TODO more things to quote | ||
| format!("\"{}\"", s.replace('\\', "\\\\").replace('\"', "\\\"")) | ||
| } | ||
|
|
||
| pub fn escape_label(l: &str) -> String { | ||
| // TODO make this proper | ||
| l.replace('.', "_dot_").replace('/', "_slash_") | ||
| } |
There was a problem hiding this comment.
These are all that survived from the old powdr-asm-utils crate.
lvella
requested review from
leonardoalt,
georgwiese,
pacheco,
chriseth and
qwang98
|
@chriseth had a point that if we ever add new frontends via assembly (like WASM), we'd need some of this code back. I guess we can always revive stuff in the future. |
|
I believe there is a binary version of WASM that is actually deployed and is easier to parse than assembly. |
|
But much of the ELF parser is reusable for any architecture that has ELF targets, so it can be split and placed on the the new crate |
|
yea, makes sense |
| run: rustup component add rust-src --toolchain nightly-2024-08-01-x86_64-unknown-linux-gnu | ||
| - name: Install riscv target | ||
| run: rustup target add riscv32imac-unknown-none-elf --toolchain nightly-2024-08-01-x86_64-unknown-linux-gnu |
There was a problem hiding this comment.
Why did we not need this before?
There was a problem hiding this comment.
We built the core libraries from source, since we needed the .s files. Now we can use the pre-built.
Then you ask: "Then why do we still need the line above, that installs the rust source?"
Because the risc0 target we use for std support is a lower tier and doesn't have pre-builds, so we still need to build std from source when using it.
| .unwrap() | ||
| .contains(&"std".into()) | ||
| .flat_map(|package| { | ||
| package["targets"] |
There was a problem hiding this comment.
where was this before? I don't see it in the removed parts
There was a problem hiding this comment.
This is new code.
The previous version used the JSON from cargo -Zunstable-options --build-plan to get details from every compilation unit and find the assembly files. But it is an unstable feature with a heavy run cost (it needs a new target directory to run, and redo a lot of work).
Now that we just need to find the executables, so we use the stable and and lightweight JSON from cargo metadata (that was already been called to figure out if this is a std or no-std build).
| // TODO: if asm path is removed, there are better ways to find the | ||
| // executable name than relying on the unstable build plan. | ||
| if produce_build_plan { | ||
| args.extend(as_ref![ | ||
| OsStr; | ||
| "-Zunstable-options", | ||
| "--build-plan" | ||
| ]); | ||
| } | ||
|
|
There was a problem hiding this comment.
@leonardoalt Here is the cargo command we don't need anymore.
Removal of a quite important feature.
Better get many approvals.