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

riscv64: Use PCRelLo12I relocation on Loads #6938

Merged
merged 4 commits into from
Aug 31, 2023

Conversation

afonso360
Copy link
Contributor

👋 Hey,

This is a small follow up to #6933 that restores our ability to use a relocation for loads that point to a label / constant pool label.

It looks like the relocation that I needed to use was already implemented so this ended up being a lot easier than I expected.

@afonso360 afonso360 added the cranelift:area:riscv64 Issues related to the RISC-V 64 backend. label Aug 31, 2023
@afonso360 afonso360 requested a review from a team as a code owner August 31, 2023 09:50
@afonso360 afonso360 requested review from elliottt and removed request for a team August 31, 2023 09:50
@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Aug 31, 2023
Comment on lines 627 to 629
(Some(base), Some(imm12), _) => (base, imm12),

(_, _, Some(label)) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These match clauses here make me pause, if a label is available this means that it's ignoring offset and base, right? Could this perhaps assert that the offset is 0 and the base is not present? (or update the match clause's pattern match).

Also could the catch-all be updated to (None, None, None) and other patterns all asserted to not exist?

My guess is that this probably works as-is with what the backend already generates, so I'm thinking that this might be a good place for a few extra assertions that no other shapes are generated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't until I stared at the implementations of get_base_register and get_label_with_sink that I realized exactly one of them will return Some for any address. This would be a little simpler maybe if there were one function that returned an enum with either a label or a register. I don't think that's necessary, but at least a comment might be nice; the updated pattern matching Some(_) made me scratch my head too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this latest commit make things slightly more readable? I'm not a big fan of this match, but I also don't want to directly match the AMode since I think that would be worse.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the extra comments are very helpful, thanks!

@afonso360 afonso360 enabled auto-merge August 31, 2023 16:44
@afonso360 afonso360 disabled auto-merge August 31, 2023 17:09
_ => {
let (addr, imm12) = match (base, offset_imm12, label) {
// When loading from a Reg+Offset, if the offset fits into an imm12 we can directly encode it.
(Some(base), Some(imm12), _) => (base, imm12),
Copy link
Member

@alexcrichton alexcrichton Aug 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this grow a None instead of _ to assert that label+base doesn't show up? Or, alternatively, could this runtime assert that label is None here?

@afonso360 afonso360 enabled auto-merge August 31, 2023 17:49
@afonso360 afonso360 added this pull request to the merge queue Aug 31, 2023
Merged via the queue into bytecodealliance:main with commit 5ec7318 Aug 31, 2023
@afonso360 afonso360 deleted the riscv-load-reloc branch August 31, 2023 19:04
pchickey pushed a commit that referenced this pull request Sep 1, 2023
…6950)

* Enhance `async` configuration of `bindgen!` macro (#6942)

This commit takes a leaf out of `wiggle`'s book to enable bindings
generation for async host functions where only some host functions are
async instead of all of them. This enhances the `async` key with a few
more options:

    async: {
        except_imports: ["foo"],
        only_imports: ["bar"],
    }

This is beyond what `wiggle` supports where either an allow-list or
deny-list can be specified (although only one can be specified). This
can be useful if either the list of sync imports or the list of async
imports is small.

* cranelift-interpreter: Fix SIMD shifts and rotates (#6939)

* cranelift-interpreter: Fix SIMD `ishl`/`{s,u}`shr

* fuzzgen: Enable a few more ops

* cranelift: Fix tests for {u,s}shr

* fuzzgen: Change pattern matching arms for shifts

Co-Authored-By: Jamey Sharp <[email protected]>

---------

Co-authored-by: Jamey Sharp <[email protected]>

* Partially revert CLI argument changes from #6737 (#6944)

* Partially revert CLI argument changes from #6737

This commit is a partial revert of #6737. That change was reverted
in #6830 for the 12.0.0 release of Wasmtime and otherwise it's currently
slated to get released with the 13.0.0 release of Wasmtime. Discussion
at today's Wasmtime meeting concluded that it's best to couple this
change with #6925 as a single release rather than spread out across
multiple releases. This commit is thus the revert of #6737, although
it's a partial revert in that I've kept many of the new tests added to
showcase the differences before/after when the change lands.

This means that Wasmtime 13.0.0 will exhibit the same CLI behavior as
12.0.0 and all prior releases. The 14.0.0 release will have both a new
CLI and new argument passing semantics. I'll revert this revert (aka
re-land #6737) once the 13.0.0 release branch is created and `main`
becomes 14.0.0.

* Update release notes

* riscv64: Use `PCRelLo12I` relocation on Loads (#6938)

* riscv64: Use `PCRelLo12I` relocation on Loads

* riscv64: Strenghten pattern matching when emitting Load's

* riscv64: Clarify some of the load address logic

* riscv64: Even stronger matching

* Update Rust in CI to 1.72.0, clarify Wasmtime's MSRV (#6900)

* Update Rust in CI to 1.72.0

* Update CI, tooling, and docs for MSRV

This commit codifies an MSRV policy for Wasmtime at "stable minus two"
meaning that the latest three releases of Rust will be supported. This
is enforced on CI with a full test suite job running on Linux x86_64
with the minimum supported Rust version. The full test suite will use
the latest stable version. A downside of this approach is that new
changes may break MSRV support on non-Linux or non-x86_64 platforms and
we won't know about it, but that's deemed a minor enough risk at this
time.

A minor fix is applied to Wasmtime's `Cargo.toml` to support Rust 1.70.0
instead of requiring Rust 1.71.0

* Fix installation of rust

* Scrape MSRV from Cargo.toml

* Cranelift is the same as Wasmtime's MSRV now, more words too

* Fix a typo

* aarch64: Use `RegScaled*` addressing modes (#6945)

This commit adds a few cases to `amode` construction on AArch64 for
using the `RegScaled*` variants of `AMode`. This won't affect wasm due
to this only matching the sign-extension happening before the shift, but
it should otherwise help non-wasm Cranelift use cases.

Closes #6742

* cranelift: Validate `iconst` ranges (#6850)

* cranelift: Validate `iconst` ranges

Add the following checks:

`iconst.i8`  immediate must be within 0 .. 2^8-1
`iconst.i16` immediate must be within 0 .. 2^16-1
`iconst.i32` immediate must be within 0 .. 2^32-1

Resolves #3059

* cranelift: Parse `iconst` according to its type

Modifies the parser for textual CLIF so that V in `iconst.T V` is
parsed according to T.

Before this commit, something like `iconst.i32 0xffff_ffff_ffff` was
valid because all `iconst` were parsed the same as an
`iconst.i64`. Now the above example will throw an error.

Also, a negative immediate as in `iconst.iN -X` is now converted to
`2^N - X`.

This commit also fixes some broken tests.

* cranelift: Update tests to match new CLIF parser

* Some minor fixes and features for WASI and sockets (#6948)

* Use `command::add_to_linker` in tests to reduce the number of times
  all the `add_to_linker` are listed.
* Add all `wasi:sockets` interfaces currently implemented to both the
  sync and async `command` functions (this enables all the interfaces in
  the CLI for example).
* Use `tokio::net::TcpStream::try_io` whenever I/O is performed on a
  socket, ensuring that readable/writable flags are set/cleared
  appropriately (otherwise once readable a socket is infinitely readable).
* Add a `with_ambient_tokio_runtime` helper function to use when
  creating a `tokio::net::TcpStream` since otherwise it panics due to a
  lack of active runtime in a synchronous context.
* Add `WouldBlock` handling to return a 0-length read.
* Add an `--inherit-network` CLI flag to enable basic usage of sockets
  in the CLI.

This will conflict a small amount with #6877 but should be easy to
resolve, and otherwise this targets different usability points/issues
than that PR.

---------

Co-authored-by: Afonso Bordado <[email protected]>
Co-authored-by: Jamey Sharp <[email protected]>
Co-authored-by: Timothée Jourde <[email protected]>
eduardomourar pushed a commit to eduardomourar/wasmtime that referenced this pull request Sep 6, 2023
* riscv64: Use `PCRelLo12I` relocation on Loads

* riscv64: Strenghten pattern matching when emitting Load's

* riscv64: Clarify some of the load address logic

* riscv64: Even stronger matching
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:riscv64 Issues related to the RISC-V 64 backend. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants