T0b1/wasmtime
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

cranelift-isle: Add "partial" flag for constructors (#5392)

Browse Source 
* cranelift-isle: Add "partial" flag for constructors

Instead of tying fallibility of constructors to whether they're either
internal or pure, this commit assumes all constructors are infallible
unless tagged otherwise with a "partial" flag.

Internal constructors without the "partial" flag are not allowed to use
constructors which have the "partial" flag on the right-hand side of any
rules, because they have no way to report last-minute match failures.

Multi-constructors should never be "partial"; they report match failures
with an empty iterator instead. In turn this means you can't use partial
constructors on the right-hand side of internal multi-constructor rules.
However, you can use the same constructors on the left-hand side with
`if` or `if-let` instead.

In many cases, ISLE can already trivially prove that an internal
constructor always returns `Some`. With this commit, those cases are
largely unchanged, except for removing all the `Option`s and `Some`s
from the generated code for those terms.

However, for internal non-partial constructors where ISLE could not
prove that, it now emits an `unreachable!` panic as the last-resort,
instead of returning `None` like it used to do. Among the existing
backends, here's how many constructors have these panic cases:

- x64: 14% (53/374)
- aarch64: 15% (41/277)
- riscv64: 23% (26/114)
- s390x: 47% (268/567)

It's often possible to rewrite rules so that ISLE can tell the panic can
never be hit. Just ensure that there's a lowest-priority rule which has
no constraints on the left-hand side.

But in many of these constructors, it's difficult to statically prove
the unhandled cases are unreachable because that's only down to
knowledge about how they're called or other preconditions.

So this commit does not try to enforce that all terms have a last-resort
fallback rule.

* Check term flags while translating expressions

Instead of doing it in a separate pass afterward.

This involved threading all the term flags (pure, multi, partial)
through the recursive `translate_expr` calls, so I extracted the flags
to a new struct so they can all be passed together.

* Validate multi-term usage

Now that I've threaded the flags through `translate_expr`, it's easy to
check this case too, so let's just do it.

* Extract `ReturnKind` to use in `ExternalSig`

There are only three legal states for the combination of `multi` and
`infallible`, so replace those fields of `ExternalSig` with a
three-state enum.

* Remove `Option` wrapper from multi-extractors too

If we'd had any external multi-constructors this would correct their
signatures as well.

* Update ISLE tests

* Tag prelude constructors as pure where appropriate

I believe the only reason these weren't marked `pure` before was because
that would have implied that they're also partial. Now that those two
states are specified separately we apply this flag more places.

* Fix my changes to aarch64 `lower_bmask` and `imm` terms
This commit is contained in:
Jamey Sharp
2022-12-07 17:16:03 -08:00
committed by GitHub
parent c9527e0af6
commit 8726eeefb3
26 changed files with 433 additions and 358 deletions
Download Patch File Download Diff File Expand all files Collapse all files

23
cranelift/codegen/src/isa/aarch64/inst.isle
View File

@@ -1599,7 +1599,7 @@
))
;; Extractors for target features ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
(decl pure sign_return_address_disabled () Unit)
(decl pure partial sign_return_address_disabled () Unit)
(extern constructor sign_return_address_disabled sign_return_address_disabled)
(decl use_lse () Inst)
@@ -1607,13 +1607,13 @@
;; Extractor helpers for various immmediate constants ;;;;;;;;;;;;;;;;;;;;;;;;;;
(decl pure imm_logic_from_u64 (Type u64) ImmLogic)
(decl pure partial imm_logic_from_u64 (Type u64) ImmLogic)
(extern constructor imm_logic_from_u64 imm_logic_from_u64)
(decl pure imm_logic_from_imm64 (Type Imm64) ImmLogic)
(decl pure partial imm_logic_from_imm64 (Type Imm64) ImmLogic)
(extern constructor imm_logic_from_imm64 imm_logic_from_imm64)
(decl pure imm_shift_from_imm64 (Type Imm64) ImmShift)
(decl pure partial imm_shift_from_imm64 (Type Imm64) ImmShift)
(extern constructor imm_shift_from_imm64 imm_shift_from_imm64)
(decl imm_shift_from_u8 (u8) ImmShift)
@@ -1672,13 +1672,13 @@
(decl imm12_from_negated_u64 (Imm12) u64)
(extern extractor imm12_from_negated_u64 imm12_from_negated_u64)
(decl pure lshr_from_u64 (Type u64) ShiftOpAndAmt)
(decl pure partial lshr_from_u64 (Type u64) ShiftOpAndAmt)
(extern constructor lshr_from_u64 lshr_from_u64)
(decl pure lshl_from_imm64 (Type Imm64) ShiftOpAndAmt)
(decl pure partial lshl_from_imm64 (Type Imm64) ShiftOpAndAmt)
(extern constructor lshl_from_imm64 lshl_from_imm64)
(decl pure lshl_from_u64 (Type u64) ShiftOpAndAmt)
(decl pure partial lshl_from_u64 (Type u64) ShiftOpAndAmt)
(extern constructor lshl_from_u64 lshl_from_u64)
(decl integral_ty (Type) Type)
@@ -1687,10 +1687,10 @@
(decl valid_atomic_transaction (Type) Type)
(extern extractor valid_atomic_transaction valid_atomic_transaction)
(decl pure is_zero_simm9 (SImm9) Unit)
(decl pure partial is_zero_simm9 (SImm9) Unit)
(extern constructor is_zero_simm9 is_zero_simm9)
(decl pure is_zero_uimm12 (UImm12Scaled) Unit)
(decl pure partial is_zero_uimm12 (UImm12Scaled) Unit)
(extern constructor is_zero_uimm12 is_zero_uimm12)
;; Helper to go directly from a `Value`, when it's an `iconst`, to an `Imm12`.
@@ -3614,8 +3614,9 @@
;; csetm res, ne
(rule 3
(lower_bmask out_ty (fits_in_16 in_ty) val)
(let ((mask_bits ImmLogic (imm_logic_from_u64 $I32 (ty_mask in_ty)))
(masked Reg (and_imm $I32 (value_regs_get val 0) mask_bits)))
; This if-let can't fail due to ty_mask always producing 8/16 consecutive 1s.
(if-let mask_bits (imm_logic_from_u64 $I32 (ty_mask in_ty)))
(let ((masked Reg (and_imm $I32 (value_regs_get val 0) mask_bits)))
(lower_bmask out_ty $I32 masked)))
;; Exceptional `lower_icmp_into_flags` rules.

4
cranelift/codegen/src/isa/aarch64/lower.isle
View File

@@ -2,7 +2,7 @@
;; The main lowering constructor term: takes a clif `Inst` and returns the
;; register(s) within which the lowered instruction's result values live.
(decl lower (Inst) InstOutput)
(decl partial lower (Inst) InstOutput)
;; Variant of the main lowering constructor term, which receives an
;; additional argument (a vector of branch targets to be used) for
@@ -12,7 +12,7 @@
;; blocks while we lower, the fallthrough in the new order is not (necessarily)
;; the same as the fallthrough in CLIF. So, we use the explicitly-provided
;; target.
(decl lower_branch (Inst VecMachLabel) InstOutput)
(decl partial lower_branch (Inst VecMachLabel) InstOutput)
;;;; Rules for `iconst` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

6
cranelift/codegen/src/isa/riscv64/inst.isle
View File

@@ -839,9 +839,9 @@
;; extend int if need.
(decl ext_int_if_need (bool ValueRegs Type) ValueRegs)
;;; for I8 and I16 ...
;;; for I8, I16, and I32 ...
(rule -1
(ext_int_if_need signed val (fits_in_32 ty))
(ext_int_if_need signed val ty)
(gen_extend val signed (ty_bits ty) 64))
;;; otherwise this is a I64 or I128
;;; no need to extend.
@@ -1870,7 +1870,7 @@
(decl vec_label_get (VecMachLabel u8) MachLabel )
(extern constructor vec_label_get vec_label_get)
(decl lower_branch (Inst VecMachLabel) InstOutput)
(decl partial lower_branch (Inst VecMachLabel) InstOutput)
(rule (lower_branch (jump _ _) targets )
(side_effect (SideEffectNoResult.Inst (gen_jump (vec_label_get targets 0)))))

2
cranelift/codegen/src/isa/riscv64/lower.isle
View File

@@ -2,7 +2,7 @@
;; The main lowering constructor term: takes a clif `Inst` and returns the
;; register(s) within which the lowered instruction's result values live.
(decl lower (Inst) InstOutput)
(decl partial lower (Inst) InstOutput)
;;;; Rules for `iconst` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

10
cranelift/codegen/src/isa/riscv64/lower/isle.rs
View File

@@ -121,7 +121,7 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, Flags, IsaFlags, 6>
targets: &VecMachLabel,
ty: Type,
) -> InstOutput {
let test = generated_code::constructor_lower_icmp(self, cc, a, b, ty).unwrap();
let test = generated_code::constructor_lower_icmp(self, cc, a, b, ty);
self.emit(&MInst::CondBr {
taken: BranchTarget::Label(targets[0]),
not_taken: BranchTarget::Label(targets[1]),
@@ -308,11 +308,11 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, Flags, IsaFlags, 6>
ValueRegs::two(shamt, len_sub_shamt)
}
fn has_b(&mut self) -> Option<bool> {
Some(self.isa_flags.has_b())
fn has_b(&mut self) -> bool {
self.isa_flags.has_b()
}
fn has_zbkb(&mut self) -> Option<bool> {
Some(self.isa_flags.has_zbkb())
fn has_zbkb(&mut self) -> bool {
self.isa_flags.has_zbkb()
}
fn valueregs_2_reg(&mut self, val: Value) -> Reg {

16
cranelift/codegen/src/isa/s390x/inst.isle
View File

@@ -1537,15 +1537,15 @@
;; Detect specific integer values
(decl pure i64_nonequal (i64 i64) i64)
(decl pure partial i64_nonequal (i64 i64) i64)
(extern constructor i64_nonequal i64_nonequal)
(decl pure i64_nonzero (i64) i64)
(decl pure partial i64_nonzero (i64) i64)
(rule (i64_nonzero x)
(if (i64_nonequal x 0))
x)
(decl pure i64_not_neg1 (i64) i64)
(decl pure partial i64_not_neg1 (i64) i64)
(rule (i64_not_neg1 x)
(if (i64_nonequal x -1))
x)
@@ -1799,11 +1799,11 @@
;; Form the sum of two offset values, and check that the result is
;; a valid `MemArg::Symbol` offset (i.e. is even and fits into i32).
(decl pure memarg_symbol_offset_sum (i64 i64) i32)
(decl pure partial memarg_symbol_offset_sum (i64 i64) i32)
(extern constructor memarg_symbol_offset_sum memarg_symbol_offset_sum)
;; Likewise, but just check a single offset value.
(decl pure memarg_symbol_offset (i64) i32)
(decl pure partial memarg_symbol_offset (i64) i32)
(rule (memarg_symbol_offset x)
(memarg_symbol_offset_sum x 0))
@@ -1837,7 +1837,7 @@
;; Test whether a `load` address will be lowered to a `MemArg::Symbol`.
(decl pure load_sym (Inst) Inst)
(decl pure partial load_sym (Inst) Inst)
(rule (load_sym inst)
(if-let (load _ (symbol_value (symbol_value_data _ (reloc_distance_near) sym_offset))
(i64_from_offset load_offset))
@@ -1845,7 +1845,7 @@
(if (memarg_symbol_offset_sum sym_offset load_offset))
inst)
(decl pure uload16_sym (Inst) Inst)
(decl pure partial uload16_sym (Inst) Inst)
(rule (uload16_sym inst)
(if-let (uload16 _ (symbol_value (symbol_value_data _ (reloc_distance_near) sym_offset))
(i64_from_offset load_offset))
@@ -2752,7 +2752,7 @@
;; Similarly, because we cannot allocate temp registers, if an instruction
;; requires matching source and destination registers, this needs to be handled
;; by the user. Another helper to verify that constraint.
(decl pure same_reg (WritableReg Reg) Reg)
(decl pure partial same_reg (WritableReg Reg) Reg)
(extern constructor same_reg same_reg)
;; Push a `MInst.AluRRR` instruction to a sequence.

4
cranelift/codegen/src/isa/s390x/lower.isle
View File

@@ -2,12 +2,12 @@
;; The main lowering constructor term: takes a clif `Inst` and returns the
;; register(s) within which the lowered instruction's result values live.
(decl lower (Inst) InstOutput)
(decl partial lower (Inst) InstOutput)
;; A variant of the main lowering constructor term, used for branches.
;; The only difference is that it gets an extra argument holding a vector
;; of branch targets to be used.
(decl lower_branch (Inst VecMachLabel) InstOutput)
(decl partial lower_branch (Inst VecMachLabel) InstOutput)
;;;; Rules for `iconst` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

18
cranelift/codegen/src/isa/s390x/lower/isle.rs
View File

@@ -526,15 +526,13 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, Flags, IsaFlags, 6>
}
#[inline]
fn lane_order(&mut self) -> Option<LaneOrder> {
Some(lane_order_for_call_conv(
self.lower_ctx.abi().call_conv(self.lower_ctx.sigs()),
))
fn lane_order(&mut self) -> LaneOrder {
lane_order_for_call_conv(self.lower_ctx.abi().call_conv(self.lower_ctx.sigs()))
}
#[inline]
fn be_lane_idx(&mut self, ty: Type, idx: u8) -> u8 {
match self.lane_order().unwrap() {
match self.lane_order() {
LaneOrder::LittleEndian => ty.lane_count() as u8 - 1 - idx,
LaneOrder::BigEndian => idx,
}
@@ -542,7 +540,7 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, Flags, IsaFlags, 6>
#[inline]
fn be_vec_const(&mut self, ty: Type, n: u128) -> u128 {
match self.lane_order().unwrap() {
match self.lane_order() {
LaneOrder::LittleEndian => n,
LaneOrder::BigEndian => {
let lane_count = ty.lane_count();
@@ -568,7 +566,7 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, Flags, IsaFlags, 6>
#[inline]
fn shuffle_mask_from_u128(&mut self, idx: u128) -> (u128, u16) {
let bytes = match self.lane_order().unwrap() {
let bytes = match self.lane_order() {
LaneOrder::LittleEndian => idx.to_be_bytes().map(|x| {
if x < 16 {
15 - x
@@ -590,7 +588,7 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, Flags, IsaFlags, 6>
let inst = self.lower_ctx.dfg().value_def(val).inst()?;
let constant = self.lower_ctx.get_constant(inst)?;
let ty = self.lower_ctx.output_ty(inst, 0);
Some(zero_extend_to_u64(constant, self.ty_bits(ty).unwrap()))
Some(zero_extend_to_u64(constant, self.ty_bits(ty)))
}
#[inline]
@@ -598,7 +596,7 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, Flags, IsaFlags, 6>
let inst = self.lower_ctx.dfg().value_def(val).inst()?;
let constant = self.lower_ctx.get_constant(inst)?;
let ty = self.lower_ctx.output_ty(inst, 0);
Some(zero_extend_to_u64(!constant, self.ty_bits(ty).unwrap()))
Some(zero_extend_to_u64(!constant, self.ty_bits(ty)))
}
#[inline]
@@ -620,7 +618,7 @@ impl generated_code::Context for IsleContext<'_, '_, MInst, Flags, IsaFlags, 6>
let inst = self.lower_ctx.dfg().value_def(val).inst()?;
let constant = self.lower_ctx.get_constant(inst)?;
let ty = self.lower_ctx.output_ty(inst, 0);
Some(sign_extend_to_u64(constant, self.ty_bits(ty).unwrap()))
Some(sign_extend_to_u64(constant, self.ty_bits(ty)))
}
#[inline]

2
cranelift/codegen/src/isa/x64/inst.isle
View File

@@ -865,7 +865,7 @@
;; A helper to both check that the `Imm64` and `Offset32` values sum to less
;; than 32-bits AND return this summed `u32` value. Also, the `Imm64` will be
;; zero-extended from `Type` up to 64 bits. This is useful for `to_amode`.
(decl pure sum_extend_fits_in_32_bits (Type Imm64 Offset32) u32)
(decl pure partial sum_extend_fits_in_32_bits (Type Imm64 Offset32) u32)
(extern constructor sum_extend_fits_in_32_bits sum_extend_fits_in_32_bits)
;;;; Amode lowering ;;;;

6
cranelift/codegen/src/isa/x64/lower.isle
View File

@@ -2,12 +2,12 @@
;; The main lowering constructor term: takes a clif `Inst` and returns the
;; register(s) within which the lowered instruction's result values live.
(decl lower (Inst) InstOutput)
(decl partial lower (Inst) InstOutput)
;; A variant of the main lowering constructor term, used for branches.
;; The only difference is that it gets an extra argument holding a vector
;; of branch targets to be used.
(decl lower_branch (Inst MachLabelSlice) InstOutput)
(decl partial lower_branch (Inst MachLabelSlice) InstOutput)
;;;; Rules for `iconst` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
@@ -1309,7 +1309,7 @@
if_true
if_false))
(decl pure all_ones_or_all_zeros (Value) bool)
(decl pure partial all_ones_or_all_zeros (Value) bool)
(rule (all_ones_or_all_zeros (and (icmp _ _ _) (value_type (multi_lane _ _)))) $true)
(rule (all_ones_or_all_zeros (and (fcmp _ _ _) (value_type (multi_lane _ _)))) $true)
(rule (all_ones_or_all_zeros (vconst (vconst_all_ones_or_all_zeros))) $true)