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

Build new instruction selector / machine-code emission backend #1174

Closed
cfallin opened this issue Jan 14, 2020 · 7 comments · Fixed by #1494
Closed

Build new instruction selector / machine-code emission backend #1174

cfallin opened this issue Jan 14, 2020 · 7 comments · Fixed by #1494
Labels
cranelift Issues related to the Cranelift code generator

Comments

@cfallin
Copy link
Member

cfallin commented Jan 14, 2020

This issue will track progress on our new instruction selector / machine-code emission work, which has been ongoing (in design and initial implementation).

The scope of the work is:

  • Build a new instruction selector, to replace the recipes and meta-DSL system that adds encodings to existing Cranelift IR instructions.
  • Develop a low-level encoding of machine instructions, with virtual registers prior to register allocation (an "IR" of sorts)
  • Interface the above to a new register allocator infrastructure, minira
  • Develop the binary-encoding pass that emits into a CodeSink from the above representation

As discussed among @sunfishcode, @julian-seward1, @bnjbvr, and others, we want to do this work in order to clean up the story for a new machine backend, as recipes had become difficult to write and maintain.

The system will be co-developed with an initial backend for ARM64 using the new interfaces.

@alexcrichton alexcrichton transferred this issue from bytecodealliance/cranelift Feb 28, 2020
@alexcrichton alexcrichton added the cranelift Issues related to the Cranelift code generator label Feb 28, 2020
arkpar pushed a commit to paritytech/wasmtime that referenced this issue Mar 4, 2020
* Add TLS support
* Add binemit and legalize tests
* Spill all caller-saved registers when necessary
@stefson
Copy link
Contributor

stefson commented Mar 5, 2020

@cfallin

I'm getting a compile error with building for aarch64-unknown-linux-gnu on current master, and found this pullrequest as the culprit when bisecting the issue. Here's the error:

   Compiling cranelift-codegen-meta v0.59.0 (/tmp/wasmtime/cranelift/codegen/meta)
   Compiling cranelift-bforest v0.59.0 (/tmp/wasmtime/cranelift/bforest)
   Compiling wasi-common v0.12.0 (/tmp/wasmtime/crates/wasi-common)
   Compiling cranelift-codegen v0.59.0 (/tmp/wasmtime/cranelift/codegen)
warning: unused import: `self::call::expand_call`
  --> cranelift/codegen/src/legalizer/mod.rs:35:5
   |
35 | use self::call::expand_call;
   |     ^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: unused import: `self::globalvalue::expand_global_value`
  --> cranelift/codegen/src/legalizer/mod.rs:36:5
   |
36 | use self::globalvalue::expand_global_value;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: unused import: `self::heap::expand_heap_addr`
  --> cranelift/codegen/src/legalizer/mod.rs:37:5
   |
37 | use self::heap::expand_heap_addr;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: unused import: `self::table::expand_table_addr`
  --> cranelift/codegen/src/legalizer/mod.rs:39:5
   |
39 | use self::table::expand_table_addr;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0599]: no variant or associated item named `X86ElfTlsGetAddr` found for type `ir::instructions::Opcode` in the current scope
    --> cranelift/codegen/src/regalloc/spilling.rs:272:45
     |
272  |             || opcode == crate::ir::Opcode::X86ElfTlsGetAddr
     |                                             ^^^^^^^^^^^^^^^^ variant or associated item not found in `ir::instructions::Opcode`
     | 
    ::: /tmp/wasmtime/target/aarch64-unknown-linux-gnu/debug/build/cranelift-codegen-dd375821e82eb469/out/opcodes.rs:1427:1
     |
1427 | pub enum Opcode {
     | --------------- variant or associated item `X86ElfTlsGetAddr` not found here

error[E0599]: no variant or associated item named `X86MachoTlsGetAddr` found for type `ir::instructions::Opcode` in the current scope
    --> cranelift/codegen/src/regalloc/spilling.rs:273:45
     |
273  |             || opcode == crate::ir::Opcode::X86MachoTlsGetAddr
     |                                             ^^^^^^^^^^^^^^^^^^ variant or associated item not found in `ir::instructions::Opcode`
     | 
    ::: /tmp/wasmtime/target/aarch64-unknown-linux-gnu/debug/build/cranelift-codegen-dd375821e82eb469/out/opcodes.rs:1427:1
     |
1427 | pub enum Opcode {
     | --------------- variant or associated item `X86MachoTlsGetAddr` not found here

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0599`.
error: could not compile `cranelift-codegen`.

It's propably worth noting that this only happens when executing cargo build from the wasmtime dir, and it doesn't when cd wasmtime/cranelift & cargo build. Still the error seems to be from cranelift.

@stefson
Copy link
Contributor

stefson commented Mar 5, 2020

verbose log from --verbose: verbose-log.gz

is it okay to discuss this matter here, or would you like to see a new issue for it?

@philipc
Copy link
Contributor

philipc commented Mar 5, 2020

@stefson This is an issue, not a pull request. I think you want bytecodealliance/cranelift#1174 instead. The PR number you are looking at is from before cranelift was moved into wasmtime. Also, I'm pretty sure cranelift doesn't support aarch64 yet, even if it happened to compile before that PR (in fact, this issue is about enabling an arm64 backend, but see also #1075). Also related, it doesn't support arm32 either (#1173), which I think you have also been trying to build for.

@stefson
Copy link
Contributor

stefson commented Mar 5, 2020

Where's the sense in arguing about issue and pullrequest please, especially since #1174 is clearly linked to this thread. It's rather unlikely that the backends work yet, but can't say for sure if they do without building them from source and do some testing.

This patch clearly regressed big time, my message is meant to inform the author about the possibly unintended breakage.

@philipc
Copy link
Contributor

philipc commented Mar 5, 2020

I was trying to be helpful and point out a misunderstanding, not argue. You want bytecodealliance/cranelift#1174, not this issue. The author of this issue was not the author of that PR.

@stefson
Copy link
Contributor

stefson commented Mar 5, 2020

it's this commit that caused the regression: 0a1bb3b

something at github seems to have been broken by integrating cranelift, making me turning up here if I try to follow the linked issue from that very commit. I'll continue to post over there.

@cfallin
Copy link
Member Author

cfallin commented Mar 5, 2020

@stefson: To clear up one additional thing: the ARM64 backend is not on mainline yet, so notwithstanding any other recent commits or other changes, one should not expect a build on ARM64 to work quite yet. We're getting close though :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants