As per discussion today, when we have a holiday (affecting any regular
attendee), we will push the meeting by a week. This does mean we
sometimes have meetings in contiguous weeks, but given the number of
topics we usually have to discuss, erring on the side of more discussion
time (rather than just canceling) is probably not a bad thing.
For the rest of this calendar year, given an otherwise regular
biweekly-on-Mondays cadence, the holiday conflicts I am aware of are: US
Memorial Day (falls on Mon May 28, pushed meeting to Mon Jun 6); US
Labor Day (falls on Mon Sept 5, pushed to Mon Sept 12). If there are any
other holidays in the below dates, I'm happy to update further!
* Run a callback when the interruption epoch is reached
Adds Store::epoch_deadline_callback. This accepts a callback which, when
invoked, can mutate the store's contents. The callback can either return
an error (in which case we trap) or return a delta which we'll use to
set the new epoch deadline.
* Add a basic test for epoch interruption callback
* Some small nits
- Remove use of &mut in the pattern match
- Return both yields and state from run_and_count_yields_or_trap in
test code and assert on them separately.
- Add a test for trapping on a state failure.
In #4143 we made ISLE compilation part of the normal build flow again,
to avoid the issues with the checked-in source. To make this acceptably
fast, we cut down dependencies of the ISLE compiler, so the "fancy"
error printing is now optional. When not included, it just prints error
messages to stderr in a list. However, this did not include file
locations. It might be nice to have this without enabling the "fancy
printing" and waiting for that to build.
Fortunately most of the plumbing for this was already present (we had it
at one point before switching to miette). This PR adds back locations to
the basic error output. It now looks like:
```
Error building ISLE files: ISLE errors:
src/isa/aarch64/inst.isle:1:1: parse error: Unexpected token Symbol("asdf")
```
* Don't attempt to track the generated clif.isle in cargo
This causes the build script to rerun every time for me.
* Put build script debug messages on stderr instead of stdout
This keeps stdout reserved for cargo build script directives
Current codegen had a number of logic errors confusing
NAND with AND WITH COMPLEMENT, and NOR with OR WITH COMPLEMENT.
Add support for the missing z15 instructions and fix logic.
The HashMap implementation is significantly simpler than the BTreeMap
implementation. Because of this switching reduces compilation time of
cranelift-isle by about 10%.
# Before
$ hyperfine --prepare "cargo clean" "cargo build"
Benchmark 1: cargo build
Time (mean ± σ): 5.221 s ± 0.094 s [User: 10.659 s, System: 0.734 s]
Range (min … max): 5.151 s … 5.420 s 10 runs
# After
$ hyperfine --prepare "cargo clean" "cargo build"
Benchmark 1: cargo build
Time (mean ± σ): 4.746 s ± 0.150 s [User: 9.109 s, System: 0.721 s]
Range (min … max): 4.630 s … 5.144 s 10 runs
This PR fixes#4066: it modifies the Cranelift `build.rs` workflow to
invoke the ISLE DSL compiler on every compilation, rather than only
when the user specifies a special "rebuild ISLE" feature.
The main benefit of this change is that it vastly simplifies the mental
model required of developers, and removes a bunch of failure modes
we have tried to work around in other ways. There is now just one
"source of truth", the ISLE source itself, in the repository, and so there
is no need to understand a special "rebuild" step and how to handle
merge errors. There is no special process needed to develop the compiler
when modifying the DSL. And there is no "noise" in the git history produced
by constantly-regenerated files.
The two main downsides we discussed in #4066 are:
- Compile time could increase, by adding more to the "meta" step before the main build;
- It becomes less obvious where the source definitions are (everything becomes
more "magic"), which makes exploration and debugging harder.
This PR addresses each of these concerns:
1. To maintain reasonable compile time, it includes work to cut down the
dependencies of the `cranelift-isle` crate to *nothing* (only the Rust stdlib),
in the default build. It does this by putting the error-reporting bits
(`miette` crate) under an optional feature, and the logging (`log` crate) under
a feature-controlled macro, and manually writing an `Error` impl rather than
using `thiserror`. This completely avoids proc macros and the `syn` build slowness.
The user can still get nice errors out of `miette`: this is enabled by specifying
a Cargo feature `--features isle-errors`.
2. To allow the user to optionally inspect the generated source, which nominally
lives in a hard-to-find path inside `target/` now, this PR adds a feature `isle-in-source-tree`
that, as implied by the name, moves the target for ISLE generated source into
the source tree, at `cranelift/codegen/isle_generated_source/`. It seems reasonable
to do this when an explicit feature (opt-in) is specified because this is how ISLE regeneration
currently works as well. To prevent surprises, if the feature is *not* specified, the
build fails if this directory exists.
In #4104 we discussed whether it makes sense for the division and
remainder ops to support vector types. We concluded that because most
hardware doesn't support it directly, it probably is not ideal to force
all backends to polyfill it. In the future we can always reverse this
decision, perhaps with a platform-independent legalization.
This PR restricts the allowed types on the CLIF ops to integer types
only.
This test was added between the last CI run on #4088 and its merge to
main, and the changes in #4088 (use of constants directly in instruction
via load from constant pool, rather than from a register initialized by
a separate instruction) cause it to fail now.
This PR alters the test to be invariant to regalloc and argument
decisions during lowering, as the test is really checking (per the
comment) that we get two cmoves without an intervening move. As such, it
just matches the instruction opcodes, irrespective of the arguments.
* Allow emitting u64 constants into constant pool.
* Use constant pool for constants on x64 that do not fit in a simm32 and are needed as a RegMem or RegMemImm.
* Fix rip-relative addressing bug in pinsrd emission.
* Narrow `allow(dead_code)` declarations
Having module wide `allow(dead_code)` may hide some code that's really
dead. In this commit I just narrowed the declarations to the specific
enum variants that were not used (as it seems reasonable to keep them
and their handling in all the matches, for future use). And the compiler
found more dead code that I think we can remove safely in the short
term.
With this, the only files annotated with a module-wide
`allow(dead_code)` are isle-generated files.
* resurrect some functions as test helpers
* ISLE compiler: fix priority-trie interval bug. (#4093)
This PR fixes a bug in the ISLE compiler related to rule priorities.
An important note first: the bug did not affect the correctness of the
Cranelift backends, either in theory (because the rules should be
correct applied in any order, even contrary to the stated priorities)
or in practice (because the generated code actually does not change at
all with the DSL compiler fix, only with a separate minimized bug
example).
The issue was a simple swap of `min` for `max` (see first
commit). This is the minimal fix, I think, to get a correct
priority-trie with the minimized bug example in this commit.
However, while debugging this, I started to convince myself that the
complexity of merging multiple priority ranges using the sort of
hybrid interval tree / string-matching trie data structure was
unneeded. The original design was built with the assumption we might
have a bunch of different priority levels, and would need the
efficiency of merging where possible. But in practice we haven't used
priorities this way: the vast majority of lowering rules exist at the
default (priority 0), and just a few overrides are explicitly at prio
1, 2 or (rarely) 3.
So, it turns out to be a lot simpler to label trie edges with (prio,
symbol) rather than (prio-range, symbol), and delete the whole mess of
interval-splitting logic on insertion. It's easier (IMHO) to convince
oneself that the resulting insertion algorithm is correct.
I was worried that this might impact the size of the generated Rust
code or its runtime, but In fact, to my initial surprise (but it makes
sense given the above "rarely used" factor), the generated code with
this compiler fix is *exactly the same*. I rebuilt with `--features
rebuild-isle,all-arch` but... there were no diffs to commit! This is
to me the simplest evidence that we didn't really need that
complexity.
* Fix earlier commit from #4093: properly sort trie.
This commit fixes an in-hindsight-obvious bug in #4093: the trie's edges
must be sorted recursively, not just at the top level.
With this fix, the generated code differs only in one cosmetic way (a
let-binding moves) but otherwise is the same.
This includes @fitzgen's fix to the CI (from the revert in #4102) that
deletes manifests to actually check that the checked-in source is
consistent with the checked-in compiler. The force-rebuild step is now
in a shell script for convenience: anyone hacking on the ISLE compiler
itself can use this script to more easily rebuild everything.
* Add note to build.rs to remind to update force-rebuild-isle.sh
This commit fixes an issue introduced in #4046 where the checks for
ensuring that the memory initialization image for a module was
constrained in its size failed to trigger and a very small module could
produce an arbitrarily large memory image.
The bug in question was that if a module only had empty data segments at
arbitrarily small and large addresses then the loop which checks whether
or not the image is allowed was skipped entirely since it was seen that
the memory had no data size. The fix here is to skip segments that are
empty to ensure that if the validation loop is skipped then no data
segments will be processed to create the image (and the module won't end
up having an image in the end).
* Revert "ISLE compiler: fix priority-trie interval bug. (#4093)"
This reverts commit 2122337112.
* ci: Delete ISLE manifest files to force an ISLE rebuild
This makes it so that we actually rebuild ISLE rather than just check that the
manifests are up-to-date.
The automated release process failed last night because the `./publish
bump` process failed to automatically increment the version number. This
is fixed in #4097 but it also seems prudent to run this continually on
CI as well to ensure that this doesn't regress again.
We won't be promising that this crate has API-compatible patch releases
and this also fixes the execution of the release process since the bump
of version numbers failed due to this being in this list.
* Remove the `Paged` memory initialization variant
This commit simplifies the `MemoryInitialization` enum by removing the
`Paged` variant. The `Paged` variant was originally added for uffd, but
that support has now been removed in #4040. This is no longer necessary
but is still used as an intermediate step of becoming a `Static` variant
of initialized memory (which copy-on-write uses). As a result this
commit largely modifies the static initialization of memory steps and
folds the two methods together.
* Apply suggestions from code review
Co-authored-by: Peter Huene <peter@huene.dev>
Co-authored-by: Peter Huene <peter@huene.dev>
As discussed previously, we need a way to be able to configure Wasmtime when running it in the Sightglass benchmark infrastructure. The easiest way to do this seemed to be to pass a string from Sightglass to the `bench-api` library and parse this in the same way that Wasmtime parses its CLI flags. The structure that contains these flags is `CommonOptions`, so it has been moved to its own crate to be depended on by both `wasmtime-cli` and `wasmtime-bench-api`. Also, this change adds an externally-visible function for parsing a string into `CommonOptions`, which is used for configuring an engine.
In #3744, we identified that extra `mov` instructions were inserted in
between the `cmov` instructions that CLIF's `select` lowers to. The
switch to regalloc2 resolved this and this test checks that no
intervening `mov`s are inserted. Closes#3744.
The pretty-printing had swapped dst and src2; this was introduced when
we moved to RA2 (sorry about that! IMHO we should do something to
automate the mapping between regalloc arg collection and pretty
printing/emission).
`src2` comes at the end because it has a variable number of register
mentions; this is in line with how many of the other inst formats work.
Actual emitted code was never incorrect, just the pretty-printing.
Updated test golden outputs look correct to me now, including the one
that we saw was incorrect in #3945.
This PR fixes a bug in the ISLE compiler related to rule priorities.
An important note first: the bug did not affect the correctness of the
Cranelift backends, either in theory (because the rules should be
correct applied in any order, even contrary to the stated priorities)
or in practice (because the generated code actually does not change at
all with the DSL compiler fix, only with a separate minimized bug
example).
The issue was a simple swap of `min` for `max` (see first
commit). This is the minimal fix, I think, to get a correct
priority-trie with the minimized bug example in this commit.
However, while debugging this, I started to convince myself that the
complexity of merging multiple priority ranges using the sort of
hybrid interval tree / string-matching trie data structure was
unneeded. The original design was built with the assumption we might
have a bunch of different priority levels, and would need the
efficiency of merging where possible. But in practice we haven't used
priorities this way: the vast majority of lowering rules exist at the
default (priority 0), and just a few overrides are explicitly at prio
1, 2 or (rarely) 3.
So, it turns out to be a lot simpler to label trie edges with (prio,
symbol) rather than (prio-range, symbol), and delete the whole mess of
interval-splitting logic on insertion. It's easier (IMHO) to convince
oneself that the resulting insertion algorithm is correct.
I was worried that this might impact the size of the generated Rust
code or its runtime, but In fact, to my initial surprise (but it makes
sense given the above "rarely used" factor), the generated code with
this compiler fix is *exactly the same*. I rebuilt with `--features
rebuild-isle,all-arch` but... there were no diffs to commit! This is
to me the simplest evidence that we didn't really need that
complexity.
This PR refactors the x64 backend address-mode lowering to use an
incremental-build approach, where it considers each node in a tree of
`iadd`s that feed into a load/store address and, at each step, builds
the best possible `Amode`. It will combine an arbitrary number of
constant offsets (an extension beyond the current rules), and can
capture a left-shifted (scaled) index in any position of the tree
(another extension).
This doesn't have any measurable performance improvement on our Wasm
benchmarks in Sightglass, unfortunately, because the IR lowered from
wasm32 will do address computation in 32 bits and then `uextend` it to
add to the 64-bit heap base. We can't quite lift the 32-bit adds to 64
bits because this loses the wraparound semantics.
(We could label adds as "expected not to overflow", and allow *those* to
be lifted to 64 bit operations; wasm32 heap address computation should
fit this. This is `add nuw` (no unsigned wrap) in LLVM IR terms. That's
likely my next step.)
Nevertheless, (i) this generalizes the cases we can handle, which should
be a good thing, all other things being equal (and in this case, no
compile time impact was measured); and (ii) might benefit non-Wasm
frontends.
Currently, we have partial Spectre mitigation: we protect heap accesses
with dynamic bounds checks. Specifically, we guard against errant
accesses on the misspeculated path beyond the bounds-check conditional
branch by adding a conditional move that is also dependent on the
bounds-check condition. This data dependency on the condition is not
speculated and thus will always pick the "safe" value (in the heap case,
a NULL address) on the misspeculated path, until the pipeline flushes
and recovers onto the correct path.
This PR uses the same technique both for table accesses -- used to
implement Wasm tables -- and for jumptables, used to implement Wasm
`br_table` instructions.
In the case of Wasm tables, the cmove picks the table base address on
the misspeculated path. This is equivalent to reading the first table
entry. This prevents loads of arbitrary data addresses on the
misspeculated path.
In the case of `br_table`, the cmove picks index 0 on the misspeculated
path. This is safer than allowing a branch to an address loaded from an
index under misspeculation (i.e., it preserves control-flow integrity
even under misspeculation).
The table mitigation is controlled by a Cranelift setting, on by
default. The br_table mitigation is always on, because it is part of the
single lowering pseudoinstruction. In both cases, the impact should be
minimal: a single extra cmove in a (relatively) rarely-used operation.
The table mitigation is architecture-independent (happens during
legalization); the br_table mitigation has been implemented for both x64
and aarch64. (I don't know enough about s390x to implement this
confidently there, but would happily review a PR to do the same on that
platform.)
This PR removes "argument polarity": the feature of ISLE extractors that lets them take
inputs aside from the value to be matched.
Cases that need this expressivity have been subsumed by #4072 with if-let clauses;
we can now finally remove this misfeature of the language, which has caused significant
confusion and has always felt like a bit of a hack.
This PR (i) removes the feature from the ISLE compiler; (ii) removes it from the reference
documentation; and (iii) refactors away all uses of the feature in our three existing
backends written in ISLE.
In #4072 I mistakenly put the subsection about if-let clauses in the
language doc just below the next section header, so it's in the wrong
section ("mapping to Rust"). This moves it back upward to where it
should be. Sorry about that!
While working with the runtime `Memory` object, it became clear that
some refactoring was needed. In order to implement shared memory from
the threads proposal, we must be able to atomically change the memory
size. Previously, the split into variants, `Memory::Static` and
`Memory::Dynamic`, made any attempt to lock forced us to duplicate logic
in various places.
This change moves `enum Memory { Static..., Dynamic... }` to simply
`struct Memory(Box<dyn RuntimeLinearMemory>)`. A new type,
`ExternalMemory`, takes the place of `Memory::Static` and also
implements the `RuntimeLinearMemory` trait, allowing `Memory` to contain
the same two options as before: `MmapMemory` for `Memory::Dynamic` and
`ExternalMemory` for `Memory::Static`. To interface with the
`PoolingAllocator`, this change also required the ability to downcast to
the internal representation.
This PR adds support for `if-let` clauses, as proposed in
bytecodealliance/rfcs#21. These clauses augment the left-hand side
(pattern-matching phase) of rules in the ISLE instruction-selection DSL
with sub-patterns matching on sub-expressions. The ability to list
additional match operations to perform, out-of-line from the original
toplevel pattern, greatly simplifies some idioms. See the RFC for more
details and examples of use.
This fixes a bug where the ISLE compiler would refuse to accept
out-of-order declarations in the order of: (i) use of an implicit
conversion backed by an extern constructor; (ii) extern declaration
for that constructor.
The issue was one of phase separation: we were capturing and noting
"extern constructor" status on terms in the same pass in which we were
typechecking and resolving implicit conversions. Given this knowledge,
the fix is straightforward: externs are picked up in a prior pass.
* Update to clap 3.0
This commit migrates all CLI commands internally used in this project
from structopt/clap2 to clap 3. The intent here is to ensure that we're
using maintained versions of the dependencies as structopt and clap 2
are less maintained nowadays. Most transitions were pretty
straightforward and mostly dealing with structopt/clap3 differences.
* Fix a number of `cargo deny` errors
This commit fixes a few errors around duplicate dependencies which
arose from the prior update to clap3. This also uses a new feature in
`deny.toml`, `skip-tree`, which allows having a bit more targeted
ignores for skips of duplicate version checks. This showed a few more
locations in Wasmtime itself where we could update some dependencies.
* Run a `cargo update` over our dependencies
This'll notably fix a `cargo audit` error where we have a pinned version
of the `regex` crate which has a CVE assigned to it.
* Update to `object` and `hashbrown` crates
Prune some duplicate versions showing up from the previous `cargo update`
Currently, a variable can be named in two different ways in an ISLE
pattern. One can write a pattern like `(T x y)` that binds the two
args of `T` with the subpatterns `x` and `y`, each of which match
anything and capture the value as a bound variable. Or, one can write
a pattern like `(T x =x)`, where the first arg pattern `x` captures
the value in `x` and the second arg pattern `=x` matches only the same
value that was already captured.
It turns out (thanks to @fitzgen for this insight here [1]) that this
distinction can actually be inferred easily: if `x` isn't bound, then
mentioning it binds it; otherwise, it matches only the already-bound
variable. There's no concern about ordering (one mention binding
vs. the other) because (i) the value is equal either way, and (ii) the
types at both sites must be the same.
This language tweak seems like it should simplify things nicely! We
can remove the `=x` syntax later if we want, but this PR doesn't do
so.
[1] https://github.com/bytecodealliance/wasmtime/pull/4071#discussion_r859111513
Also fix and extend the current implementation:
- AtomicRMWOp::Clr != AtomicRmwOp::And, as the input needs to be
inverted first.
- Inputs to the cmp for the RMWLoop case are sign-extended when
needed.
- Lower Xchg to Swp.
- Lower Sub to Add with a negated input.
- Added more runtests.
Copyright (c) 2022, Arm Limited.
