Uncovered by @bjorn3 (thanks!): 8- and 16-bit rotates were not working
properly in recent versions of Cranelift with part of the lowering
migrated to ISLE.
This PR fixes a few issues:
- 8- and 16-bit rotate-left needs to mask a constant amount, if any,
because we use a 32-bit rotate instruction and so don't get the
appropriate shift-amount masking for free from x86 semantics.
- `operand_size_from_type` was incorrect: it only handled 32- and 64-bit
types and silently returned `OperandSize::Size32` for everything else.
Now uses the `OperandSize::from_ty(ty)` helper as the pre-ISLE code
did.
Our test coverage for narrow value types is not great; this PR adds some
runtests for rotl/rotr but more would always be better!
* aarch64: Migrate some bit-ops to ISLE
This commit migrates these instructions to ISLE:
* `bnot`
* `band`
* `bor`
* `bxor`
* `band_not`
* `bor_not`
* `bxor_not`
The translations were relatively straightforward but the interesting
part here was trying to reduce the duplication between all these
instructions. I opted for a route that's similar to what the lowering
does today, having a `decl` which takes the `ALUOp` and then performs
further pattern matching internally. This enabled each instruction's
lowering to be pretty simple while we still get to handle all the fancy
cases of shifts, constants, etc, for each instruction.
* Actually delete previous lowerings
* Remove dead code
This commit migrates the sign/zero extension instructions from
`lower_inst.rs` to ISLE. There's actually a fair amount going on in this
migration since a few other pieces needed touching up along the way as
well:
* First is the actual migration of `uextend` and `sextend`. These
instructions are relatively simple but end up having a number of special
cases. I've attempted to replicate all the cases here but
double-checks would be good.
* This commit actually fixes a few issues where if the result of a vector
extraction is sign/zero-extended into i128 that actually results in
panics in the current backend.
* This commit adds exhaustive testing for
extension-of-a-vector-extraction is a noop wrt extraction.
* A bugfix around ISLE glue was required to get this commit working,
notably the case where the `RegMapper` implementation was trying to
map an input to an output (meaning ISLE was passing through an input
unmodified to the output) wasn't working. This requires a `mov`
instruction to be generated and this commit updates the glue to do
this. At the same time this commit updates the ISLE glue to share more
infrastructure between x64 and aarch64 so both backends get this fix
instead of just aarch64.
Overall I think that the translation to ISLE was a net benefit for these
instructions. It's relatively obvious what all the cases are now unlike
before where it took a few reads of the code and some boolean switches
to figure out which path was taken for each flavor of input. I think
there's still possible improvements here where, for example, the
`put_in_reg_{s,z}ext64` helper doesn't use this logic so technically
those helpers could also pattern match the "well atomic loads and vector
extractions automatically do this for us" but that's a possible future
improvement for later (and shouldn't be too too hard with some ISLE
refactoring).
* aarch64: Migrate {s,u}{div,rem} to ISLE
This commit migrates four different instructions at once to ISLE:
* `sdiv`
* `udiv`
* `srem`
* `urem`
These all share similar codegen and center around the `div` instruction
to use internally. The main feature of these was to model the manual
traps since the `div` instruction doesn't trap on overflow, instead
requiring manual checks to adhere to the semantics of the instruction
itself.
While I was here I went ahead and implemented an optimization for these
instructions when the right-hand-side is a constant with a known value.
For `udiv`, `srem`, and `urem` if the right-hand-side is a nonzero
constant then the checks for traps can be skipped entirely. For `sdiv`
if the constant is not 0 and not -1 then additionally all checks can be
elided. Finally if the right-hand-side of `sdiv` is -1 the zero-check is
elided, but it still needs a check for `i64::MIN` on the left-hand-side
and currently there's a TODO where `-1` is still checked too.
* Rebasing and review conflicts
* x64: expand FloatCC enum in ISLE
* isle: regenerate manifests
* isle: generate all enum fields in `clif.isle`
This expands the `gen_isle` function to write all of the immediate
`enum`s out explicitly in `clif.isle`. Non-`enum` immediates are still
`extern primitive`.
* Only compile `enum_values` with `rebuild-isle` feature
* Only compile `gen_enum_isle` with `rebuild-isle` feature
This register is not initialized, but we protect against its being used
by never allowing an iflags/fflags-typed value to be used with
`put_value_in_regs`. All `iflags`/`fflags` usages should be handled by
pattern-matching: e.g., `trapif` explicitly matches an `iadd_ifcout`
input.
Eventually (#3249) we need to simplify this by removing
iflags/fflags-tyepd values and using bool flags instead,
pattern-matching to get the same efficient lowerings as today. For now,
this allows the ISLE assertions to pass.
This starts moving over some sign/zero-extend helpers also present in
lowering in Rust. Otherwise this is a relatively unsurprising transition
with the various cases of the instructions mapping well to ISLE
utilities.
This commit migrates the `imul` clif instruction lowering for AArch64 to
ISLE. This is a relatively complicated instruction with lots of special
cases due to the simd proposal for wasm. Like x64, however, the special
casing lends itself to ISLE quite well and the lowerings here in theory
are pretty straightforward.
The main gotcha of this commit is that this encounters a unique
situation which hasn't been encountered yet with other lowerings, namely
the `Umlal32` instruction used in the implementation of `i64x2.mul` is
unique in the `VecRRRLongOp` class of instructions in that it both reads
and writes the destination register (`use_mod` instead of simply
`use_def`). This meant that I needed to add another helper in ISLe for
creating a `vec_rrrr_long` instruction (despite this enum variant not
actually existing) which implicitly moves the first operand into the
destination before issuing the actual `VecRRRLong` instruction.
The comment says the enum is "likely to grow" and the function's been in libc since C89, so hopefully this is ok.
I'd like to use it for emitting things like array equality.
In [this
comment](https://github.com/bytecodealliance/wasmtime/pull/3545#discussion_r756284757)
I noted a potential subtle issue with the way that a few rules were
written that is fine now but could cause some unexpected pain when we
get around to verification.
Specifically, a set of rules of the form
```
(rule (A (B _)) (C))
(rule (A _) (D))
```
should, under any reasonable "default" rule ordering scheme, fire the
more specific rule `(A (B _))` when applicable, in preference to the
second "fallback" rule.
However, for future verification-specific applications of ISLE, we want
to ensure the property that a rule's meaning/validity is not dependent
on being overridden by more specific rules. In other words, if a rule
specifies a rewrite, that rewrite should always be correct; and choosing
a more specific rule can give a *better* compilation (better generated
code) but should not be necessary for correctness.
This is an admittedly under-documented part of the language, though in the
pending #3560 I added a note about rule ordering being a heuristic that
should hopefully make this slightly clearer. Ultimately I want to have
tests that choose non-default rule orderings and differentially fuzz in
order to be sure that we're following this principle; and of course once
we're actually doing verification, we'll catch issues like this upfront.
Apologies for the subtle footgun here and hopefully the reasoning is
clear enough :-)
As reported in #3173, the `select` instruction fails an assertion when it is given `v128` types as operands. This change relaxes the assertion to allow the same type of XMM move that occurs for the f32 and f64 types. This fixes#3173 in the old `lower.rs` code temporarily until the relatively complex `select` lowering can be ported to ISLE.
This commit is the first "meaty" instruction added to ISLE for the
AArch64 backend. I chose to pick the first two in the current lowering's
`match` statement, `isub` and `iadd`. These two turned out to be
particularly interesting for a few reasons:
* Both had clearly migratable-to-ISLE behavior along the lines of
special-casing per type. For example 128-bit and vector arithmetic
were both easily translateable.
* The `iadd` instruction has special cases for fusing with a
multiplication to generate `madd` which is expressed pretty easily in
ISLE.
* Otherwise both instructions had a number of forms where they attempted
to interpret the RHS as various forms of constants, extends, or
shifts. There's a bit of a design space of how best to represent this
in ISLE and what I settled on was to have a special case for each form
of instruction, and the special cases are somewhat duplicated between
`iadd` and `isub`. There's custom "extractors" for the special cases
and instructions that support these special cases will have an
`rule`-per-case.
Overall I think the ISLE transitioned pretty well. I don't think that
the aarch64 backend is going to follow the x64 backend super closely,
though. For example the x64 backend is having a helper-per-instruction
at the moment but with AArch64 it seems to make more sense to only have
a helper-per-enum-variant-of-`MInst`. This is because the same
instruction (e.g. `ALUOp::Sub32`) can be expressed with multiple
different forms depending on the payload.
It's worth noting that the ISLE looks like it's a good deal larger than
the code actually being removed from lowering as part of this commit. I
think this is deceptive though because a lot of the logic in
`put_input_in_rse_imm12_maybe_negated` and `alu_inst_imm12` is being
inlined into the ISLE definitions for each instruction instead of having
it all packed into the helper functions. Some of the "boilerplate" here
is the addition of various ISLE utilities as well.
This also fixes a bug where `movsd` was incorrectly used with a memory
operand for `insertlane`, causing it to actually zero the upper bits
instead of preserving them.
Note that the insertlane logic still exists in `lower.rs` because it's
used as a helper for a few other instruction lowerings which aren't
migrated to ISLE yet. This commit also adds a helper in ISLE itself for
those other lowerings to use when they get implemented.
Closes#3216
* aarch64: Initial work to transition backend to ISLE
This commit is what is hoped to be the initial commit towards migrating
the aarch64 backend to ISLE. There's seemingly a lot of changes here but
it's intended to largely be code motion. The current thinking is to
closely follow the x64 backend for how all this is handled and
organized.
Major changes in this PR are:
* The `Inst` enum is now defined in ISLE. This avoids having to define
it in two places (once in Rust and once in ISLE). I've preserved all
the comments in the ISLE and otherwise this isn't actually a
functional change from the Rust perspective, it's still the same enum
according to Rust.
* Lots of little enums and things were moved to ISLE as well. As with
`Inst` their definitions didn't change, only where they're defined.
This will give future ISLE PRs access to all these operations.
* Initial code for lowering `iconst`, `null`, and `bconst` are
implemented. Ironically none of this is actually used right now
because constant lowering is handled in `put_input_in_regs` which
specially handles constants. Nonetheless I wanted to get at least
something simple working which shows off how to special case various
things that are specific to AArch64. In a future PR I plan to hook up
const-lowering in ISLE to this path so even though
`iconst`-the-clif-instruction is never lowered this should use the
const lowering defined in ISLE rather than elsewhere in the backend
(eventually leading to the deletion of the non-ISLE lowering).
* The `IsleContext` skeleton is created and set up for future additions.
* Some code for ISLE that's shared across all backends now lives in
`isle_prelude_methods!()` and is deduplicated between the AArch64
backend and the x64 backend.
* Register mapping is tweaked to do the same thing for AArch64 that it
does for x64. Namely mapping virtual registers is supported instead of
just virtual to machine registers.
My main goal with this PR was to get AArch64 into a place where new
instructions can be added with relative ease. Additionally I'm hoping to
figure out as part of this change how much to share for ISLE between
AArch64 and x64 (and other backends).
* Don't use priorities with rules
* Update .gitattributes with concise syntax
* Deduplicate some type definitions
* Rebuild ISLE
* Move isa::isle to machinst::isle
Peepmatic was an early attempt at a DSL for peephole optimizations, with the
idea that maybe sometime in the future we could user it for instruction
selection as well. It didn't really pan out, however:
* Peepmatic wasn't quite flexible enough, and adding new operators or snippets
of code implemented externally in Rust was a bit of a pain.
* The performance was never competitive with the hand-written peephole
optimizers. It was *very* size efficient, but that came at the cost of
run-time efficiency. Everything was table-based and interpreted, rather than
generating any Rust code.
Ultimately, because of these reasons, we never turned Peepmatic on by default.
These days, we just landed the ISLE domain-specific language, and it is better
suited than Peepmatic for all the things that Peepmatic was originally designed
to do. It is more flexible and easy to integrate with external Rust code. It is
has better time efficiency, meeting or even beating hand-written code. I think a
small part of the reason why ISLE excels in these things is because its design
was informed by Peepmatic's failures. I still plan on continuing Peepmatic's
mission to make Cranelift's peephole optimizer passes generated from DSL rewrite
rules, but using ISLE instead of Peepmatic.
Thank you Peepmatic, rest in peace!
- The Windows line-ending canonicalization was incomplete: we need to
canonicalize the manifest text itself too!
- The "meta deterministic check" runs the cranelift-codegen build script
N times outside of the source tree, examining what it produces to
ensure the output is always the same (is detministic). This works fine
when everything comes from the internal DSL, but when reading ISLE,
this breaks because we no longer have the ISLE source paths.
The initial ISLE integration did not hit this because without the
`rebuild-isle` feature, it simply did nothing in the build script;
now, with the manifest check, we hit the issue.
The fix for now is just to turn off all ISLE-specific behavior in the
build script by setting a special-purpose Cargo feature in the
specific CI job. Eventually IMHO we should remove or find a better way
to do this check.
Currently the lowering for `iconst` will sign-extend the payload value
of the `iconst` instruction itself, but the payload is already
sign-extended to this isn't necessary. This commit removes the redundant
sign extension.
Currently, the `build.rs` script that generates Rust source from the
ISLE DSL will only do this generation if the `rebuild-isle` Cargo
feature is specified. By default, it is not. This is based on the
principle that we (the build script) do not modify the source tree as
managed by git; git-managed files are strictly a human-managed and
human-edited resource. By adding the opt-in Cargo feature, a developer
is requesting the build script to perform an explicit action. (In my
understanding at least, this principle comes from the general philosophy
of hermetic builds: the output should be a pure function of the input,
and part of this is that the input is read-only. If we modify the source
tree, then all bets are off.)
Unfortunately, requiring the opt-in feature also creates a footgun that
is easy to hit: if a developer modifies the ISLE DSL source, but forgets
to specify the Cargo feature, then the compiler will silently be built
successfully with stale source, and will silently exclude any changes
that were made.
The generated source is checked into git for a good reason: we want DSL
compiler to not affect build times for the overwhelmingly common case
that Cranelift is used as a dependency but the backends are not being
actively developed. (This overhead comes mainly from building `islec`
itself.)
So, what to do? This PR implements a middle ground first described in
[this conversation](https://github.com/bytecodealliance/wasmtime/pull/3506#discussion_r743113351), in which we:
- Generate a hash (SHA-512) of the ISLE DSL source and produce a
"manifest" of ISLE inputs alongside the generated source; and
- Always read the ISLE DSL source, and see if the manifest is still
valid, on builds that do not have the opt-in "rebuild" feature.
This allows us to know whether the ISLE compiler output would have been
the same (modulo changes to the DSL compiler itself, which are
out-of-scope here), without actually building the ISLE compiler and
running it.
If the compiler-backend developer modifies an ISLE source file and then
tries to build `cranelift-codegen` without adding the `rebuild-isle`
Cargo feature, they get the following output:
```text
Error: the ISLE source files that resulted in the generated Rust source
* src/isa/x64/lower/isle/generated_code.rs
have changed but the generated source was not rebuilt! These ISLE source
files are:
* src/clif.isle
* src/prelude.isle
* src/isa/x64/inst.isle
* src/isa/x64/lower.isle
Please add `--features rebuild-isle` to your `cargo build` command
if you wish to rebuild the generated source, then include these changes
in any git commits you make that include the changes to the ISLE.
For example:
$ cargo build -p cranelift-codegen --features rebuild-isle
(This build script cannot do this for you by default because we cannot
modify checked-into-git source without your explicit opt-in.)
```
which will tell them exactly what they need to do to fix the problem!
Note that I am leaving the "Rebuild ISLE" CI job alone for now, because
otherwise, we are trusting whomever submits a PR to generate the correct
generated source. In other words, the manifest is a communication from
the checked-in tree to the developer, but we still need to verify that
the checked-in generated Rust source and the manifest are correct with
respect to the checked-in ISLE source.
This moves the `f32const` and `f64const` instructions from `lower.rs` to
ISLE. I was originally going to add something else but due to the
`isle.rs`'s manual use of `constructor_imm(..)` it necessitated filling
out the `imm` cases for f32/f64 constants, so I figured I'd go ahead and
move these instructions as well.
The special case for 0 constants which use `xorp{s,d}` is preserved from
`lower.rs` today, but a special case isn't added for the all-ones
constant. The comment says to use `cmpeqp{s,d}` but as discovered on
other recent PRs that's not quite sufficient because comparing a
register against itself which happens to be NaN wouldn't work, so
something else will be required (perhaps `pcmpeq` or similar? I figured
I'd defer to later)
