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

cranelift-codegen fails to compile without x86 support #1240

Closed
iximeow opened this issue Mar 5, 2020 · 5 comments
Closed

cranelift-codegen fails to compile without x86 support #1240

iximeow opened this issue Mar 5, 2020 · 5 comments
Labels
bug Incorrect behavior in the current implementation that needs fixing cranelift Issues related to the Cranelift code generator

Comments

@iximeow
Copy link
Contributor

iximeow commented Mar 5, 2020

  • What are the steps to reproduce the issue?
  • What do you expect to happen?
    • Compilation should succeed.
  • What does actually happen?
    • Compilation does not succeed.
  • Which Cranelift version / commit hash / branch are you using?
    • 3179dcf (master)
  • If relevant, can you include some extra information about your environment?
    • n/a - reproducible anywhere Cargo runs.

This was first mentioned by @stefson in this comment.

I agree with @bjorn3's assessment in reply. Since TLS access assumes a C ABI, would it make sense to just add is_call as an attribute to these opcodes? Edit: this depends on other questions like, can TlsGetAddr be hoisted in LICM? Eligible for DCE? And interaction with the verifier? Given that it includes a call I think this might be the right way to go about it. And since the call takes an argument in rdi, if there needs to be a signature for verifier reasons, it looks like we can synthesize one of the form (rdi) -> rax?

A wider concern: does anyone have ideas on how we'd build Cranelift under these different, more specific, feature flag combinations? I was surprised to learn cargo build --no-default-features --features std didn't work to show this, but that seems to be a consequence of this issue. It seems appropriate to have some test crates that build under feature flag combinations we want to support, so issues like this get caught in CI. It would raise build times a bit, but I'm not sure how much. Does that sound acceptable?

@iximeow iximeow added bug Incorrect behavior in the current implementation that needs fixing cranelift Issues related to the Cranelift code generator labels Mar 5, 2020
@bnjbvr
Copy link
Member

bnjbvr commented Mar 6, 2020

This is now biting us in Spidermonkey too, where we only build the relevant backends, so this is high priority for us.

We could add supplementary builds with disabled features in automation, to make sure they don't get broken anymore.

@bjorn3
Copy link
Contributor

bjorn3 commented Mar 6, 2020

I initially tried adding is_call, but that requires analyze_call to return a valid CallInfo. As the instructions in question are not normal calls and don't have a Signature, it is not possible to return a valid CallInfo.

can TlsGetAddr be hoisted in LICM? Eligible for DCE?

Yes and yes, I think.

And since the call takes an argument in rdi, if there needs to be a signature for verifier reasons, it looks like we can synthesize one of the form (rdi) -> rax?

How would that signature get assigned to the instruction? It is not possible to lower the instructions to libcalls, as they require a very specific combination of x86 instructions for the linker to handle it correctly.

was surprised to learn cargo build --no-default-features --features std didn't work to show this, but that seems to be a consequence of this issue.

During compilation the build script detects the current architecture and automatically enables support for it. (at least if there are no archs enabled through feature flags)

@bnjbvr
Copy link
Member

bnjbvr commented Mar 6, 2020

So my current thinking is the following: since this can't be emulated properly with a call, maybe the proper way here is to add a new accessor on the opcode? I've got a wip patch that does this, with a new property called "clobbers_all_regs", and which is tested during register allocation in place of the opcode comparisons. I thought about other names, like "emulates_call", but this seems to be the most explicit, at the very least.

@bjorn3
Copy link
Contributor

bjorn3 commented Mar 6, 2020

I've got a wip patch that does this, with a new property called "clobbers_all_regs"

👍

@bjorn3
Copy link
Contributor

bjorn3 commented Mar 6, 2020

Was actually working on the same, but named it clobbers_all :)

bjorn3 added a commit to bjorn3/wasmtime that referenced this issue Mar 6, 2020
bnjbvr added a commit to bnjbvr/wasmtime that referenced this issue Mar 6, 2020
bnjbvr added a commit to bnjbvr/wasmtime that referenced this issue Mar 17, 2020
bnjbvr added a commit to bnjbvr/wasmtime that referenced this issue Mar 23, 2020
@bnjbvr bnjbvr closed this as completed in 1d5a678 Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior in the current implementation that needs fixing cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

No branches or pull requests

3 participants