Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

aarch64: Migrate {s,u}{div,rem} to ISLE #3572

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions cranelift/codegen/src/isa/aarch64/inst.isle
Original file line number Diff line number Diff line change
Expand Up @@ -1307,6 +1307,12 @@
(decl imm12_from_u64 (Imm12) u64)
(extern extractor imm12_from_u64 imm12_from_u64)

(decl u8_into_uimm5 (u8) UImm5)
(extern constructor u8_into_uimm5 u8_into_uimm5)

(decl u8_into_imm12 (u8) Imm12)
(extern constructor u8_into_imm12 u8_into_imm12)

(decl imm12_from_negated_u64 (Imm12) u64)
(extern extractor imm12_from_negated_u64 imm12_from_negated_u64)

Expand Down Expand Up @@ -1339,6 +1345,15 @@
(decl get_extended_op (ExtendedValue) ExtendOp)
(extern constructor get_extended_op get_extended_op)

(decl nzcv (bool bool bool bool) NZCV)
(extern constructor nzcv nzcv)

(decl cond_br_zero (Reg) CondBrKind)
(extern constructor cond_br_zero cond_br_zero)

(decl cond_br_cond (Cond) CondBrKind)
(extern constructor cond_br_cond cond_br_cond)

;; Instruction creation helpers ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

;; Emit an instruction.
Expand All @@ -1352,6 +1367,9 @@
(decl zero_reg () Reg)
(extern constructor zero_reg zero_reg)

(decl writable_zero_reg () WritableReg)
(extern constructor writable_zero_reg writable_zero_reg)

;; Helper for emitting `MInst.MovZ` instructions.
(decl movz (MoveWideConst OperandSize) Reg)
(rule (movz imm size)
Expand Down Expand Up @@ -1543,3 +1561,41 @@

;; 64-bit passthrough.
(rule (put_in_reg_zext64 val @ (value_type $I64)) (put_in_reg val))

;; Misc instruction helpers ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(decl trap_if_zero_divisor (Reg) Reg)
(rule (trap_if_zero_divisor reg)
(let ((_ Unit (emit (MInst.TrapIf (cond_br_zero reg) (trap_code_division_by_zero)))))
reg))

(decl size_from_ty (Type) OperandSize)
(rule (size_from_ty (fits_in_32 _ty)) (OperandSize.Size32))
(rule (size_from_ty $I64) (OperandSize.Size64))

;; Check for signed overflow. The only case is min_value / -1.
;; The following checks must be done in 32-bit or 64-bit, depending
;; on the input type.
(decl trap_if_div_overflow (Type Reg Reg) Reg)
(rule (trap_if_div_overflow ty x y)
(let (
;; Check RHS is -1.
(_1 Unit (emit (MInst.AluRRImm12 (adds_op ty) (writable_zero_reg) y (u8_into_imm12 1))))

;; Check LHS is min_value, by subtracting 1 and branching if
;; there is overflow.
(_2 Unit (emit (MInst.CCmpImm (size_from_ty ty)
x
(u8_into_uimm5 1)
(nzcv $false $false $false $false)
Copy link
Member

Choose a reason for hiding this comment

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

Just a drive-by comment (I'm happy to review the whole thing too but wanted to note this first): eek, a language wart appears! I had included #t and #f bool constants (Scheme-style) but they lower to 1 / 0 (i.e., integers); this is largely because the DSL compiler's IR has ConstInt but not ConstBool because... no good reason. I see here you've used the $ syntax to just pass through false as an opaque constant name; that works but we should really fix this.

(Outside the scope of this PR obviously, just noting -- I'll file an issue for it. Sorry about this!)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ok! I'm happy to switch to whatever is the right way to do this, I think there's other places that stem from

(extern const $true bool)
(extern const $false bool)
which would need an update as well.

Copy link
Member

Choose a reason for hiding this comment

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

Filed #3573 for this, happy to do the cleanup later; this bit can land as-is for now :-)

(Cond.Eq))))
(_3 Unit (emit (MInst.TrapIf (cond_br_cond (Cond.Vs))
(trap_code_integer_overflow))))
)
x))

;; Helper to use either a 32 or 64-bit adds depending on the input type.
(decl adds_op (Type) ALUOp)
(rule (adds_op (fits_in_32 _ty)) (ALUOp.AddS32))
(rule (adds_op $I64) (ALUOp.AddS64))

104 changes: 104 additions & 0 deletions cranelift/codegen/src/isa/aarch64/lower.isle
Original file line number Diff line number Diff line change
Expand Up @@ -398,3 +398,107 @@
)
(value_reg result)))

;;;; Rules for `udiv` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

;; TODO: Add UDiv32 to implement 32-bit directly, rather
;; than extending the input.
;;
;; Note that aarch64's `udiv` doesn't trap so to respect the semantics of
;; CLIF's `udiv` the check for zero needs to be manually performed.
(rule (lower (has_type (fits_in_64 ty) (udiv x y)))
(value_reg (alu_rrr (ALUOp.UDiv64)
(put_in_reg_zext64 x)
(put_nonzero_in_reg_zext64 y))))

;; Helper for placing a `Value` into a `Reg` and validating that it's nonzero.
(decl put_nonzero_in_reg_zext64 (Value) Reg)
(rule (put_nonzero_in_reg_zext64 val)
(trap_if_zero_divisor (put_in_reg_zext64 val)))

;; Special case where if a `Value` is known to be nonzero we can trivially
;; move it into a register.
(rule (put_nonzero_in_reg_zext64 (and (value_type ty)
(def_inst (iconst (nonzero_u64_from_imm64 n)))))
(imm ty n))

;;;; Rules for `sdiv` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

;; TODO: Add SDiv32 to implement 32-bit directly, rather
;; than extending the input.
;;
;; The sequence of checks here should look like:
;;
;; cbnz rm, #8
;; udf ; divide by zero
;; cmn rm, 1
;; ccmp rn, 1, #nzcv, eq
;; b.vc #8
;; udf ; signed overflow
;;
;; Note The div instruction does not trap on divide by zero or overflow, so
;; checks need to be manually inserted.
;;
;; TODO: if `y` is -1 then a check that `x` is not INT_MIN is all that's
;; necessary, but right now `y` is checked to not be -1 as well.
(rule (lower (has_type (fits_in_64 ty) (sdiv x y)))
(let (
(x64 Reg (put_in_reg_sext64 x))
(y64 Reg (put_nonzero_in_reg_sext64 y))
(valid_x64 Reg (trap_if_div_overflow ty x64 y64))
(result Reg (alu_rrr (ALUOp.SDiv64) valid_x64 y64))
)
(value_reg result)))

;; Helper for extracting an immediate that's not 0 and not -1 from an imm64.
(decl safe_divisor_from_imm64 (u64) Imm64)
(extern extractor safe_divisor_from_imm64 safe_divisor_from_imm64)

;; Special case for `sdiv` where no checks are needed due to division by a
;; constant meaning the checks are always passed.
(rule (lower (has_type (fits_in_64 ty) (sdiv x (def_inst (iconst (safe_divisor_from_imm64 y))))))
(value_reg (alu_rrr (ALUOp.SDiv64)
(put_in_reg_sext64 x)
(imm ty y))))

;; Helper for placing a `Value` into a `Reg` and validating that it's nonzero.
(decl put_nonzero_in_reg_sext64 (Value) Reg)
(rule (put_nonzero_in_reg_sext64 val)
(trap_if_zero_divisor (put_in_reg_sext64 val)))

;; Note that this has a special case where if the `Value` is a constant that's
;; not zero we can skip the zero check.
(rule (put_nonzero_in_reg_sext64 (and (value_type ty)
(def_inst (iconst (nonzero_u64_from_imm64 n)))))
(imm ty n))

;;;; Rules for `urem` and `srem` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

;; Remainder (x % y) is implemented as:
;;
;; tmp = x / y
;; result = x - (tmp*y)
;;
;; use 'result' for tmp and you have:
;;
;; cbnz y, #8 ; branch over trap
;; udf ; divide by zero
;; div rd, x, y ; rd = x / y
;; msub rd, rd, y, x ; rd = x - rd * y

(rule (lower (has_type (fits_in_64 ty) (urem x y)))
(let (
(x64 Reg (put_in_reg_zext64 x))
(y64 Reg (put_nonzero_in_reg_zext64 y))
(div Reg (alu_rrr (ALUOp.UDiv64) x64 y64))
(result Reg (alu_rrrr (ALUOp3.MSub64) div y64 x64))
)
(value_reg result)))

(rule (lower (has_type (fits_in_64 ty) (srem x y)))
(let (
(x64 Reg (put_in_reg_sext64 x))
(y64 Reg (put_nonzero_in_reg_sext64 y))
(div Reg (alu_rrr (ALUOp.SDiv64) x64 y64))
(result Reg (alu_rrrr (ALUOp3.MSub64) div y64 x64))
)
(value_reg result)))
39 changes: 35 additions & 4 deletions cranelift/codegen/src/isa/aarch64/lower/isle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ pub mod generated_code;

// Types that the generated ISLE code uses via `use super::*`.
use super::{
zero_reg, AMode, ASIMDFPModImm, ASIMDMovModImm, AtomicRmwOp, BranchTarget, CallIndInfo,
CallInfo, Cond, CondBrKind, ExtendOp, FPUOpRI, Imm12, ImmLogic, ImmShift, Inst as MInst,
JTSequenceInfo, MachLabel, MoveWideConst, NarrowValueMode, Opcode, OperandSize, PairAMode, Reg,
ScalarSize, ShiftOpAndAmt, UImm5, VectorSize, NZCV,
writable_zero_reg, zero_reg, AMode, ASIMDFPModImm, ASIMDMovModImm, AtomicRmwOp, BranchTarget,
CallIndInfo, CallInfo, Cond, CondBrKind, ExtendOp, FPUOpRI, Imm12, ImmLogic, ImmShift,
Inst as MInst, JTSequenceInfo, MachLabel, MoveWideConst, NarrowValueMode, Opcode, OperandSize,
PairAMode, Reg, ScalarSize, ShiftOpAndAmt, UImm5, VectorSize, NZCV,
};
use crate::isa::aarch64::settings as aarch64_settings;
use crate::machinst::isle::*;
Expand Down Expand Up @@ -244,4 +244,35 @@ where
fn emit(&mut self, inst: &MInst) -> Unit {
self.emitted_insts.push(inst.clone());
}

fn cond_br_zero(&mut self, reg: Reg) -> CondBrKind {
CondBrKind::Zero(reg)
}

fn cond_br_cond(&mut self, cond: &Cond) -> CondBrKind {
CondBrKind::Cond(*cond)
}
Comment on lines +248 to +254
Copy link
Member

Choose a reason for hiding this comment

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

Should CondBrKind be moved into ISLE? This seems like a lot of code to write to glue the non-ISLE definition in with the ISLE code. Doesn't need to happen in this PR (or at all) unless you want, but just something I though of when looking at this code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently CondBrKind is pretty heavily used it looks like in the backend and since ISLE can only generate named-field enums instead of tuple-enums like CondBrKind is today I think I'll leave it defined externally, but I agree this'd be a good future cleanup!


fn nzcv(&mut self, n: bool, z: bool, c: bool, v: bool) -> NZCV {
NZCV::new(n, z, c, v)
}

fn u8_into_uimm5(&mut self, x: u8) -> UImm5 {
UImm5::maybe_from_u8(x).unwrap()
}

fn u8_into_imm12(&mut self, x: u8) -> Imm12 {
Imm12::maybe_from_u64(x.into()).unwrap()
}

fn writable_zero_reg(&mut self) -> WritableReg {
writable_zero_reg()
}

fn safe_divisor_from_imm64(&mut self, val: Imm64) -> Option<u64> {
match val.bits() {
0 | -1 => None,
n => Some(n as u64),
}
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
src/clif.isle be1359b4b6b153f378517c1dd95cd80f4a6bed0c7b86eaba11c088fd71b7bfe80a3c868ace245b2da0bfbbd6ded262ea9576c8e0eeacbf89d03c34a17a709602
src/prelude.isle 9bd1fcb6a3604a24cf2e05e6b7eb04dcb3b9dc8fa9a2f1c8f29c25b6e3bf7f679b3b1b72dff87501497b72bc30fc92fd755b898db7e03f380235fae931b6a74b
src/isa/aarch64/inst.isle 6e042ec14166fceae4b7133f681fdf604e20a2997e1d60f797e40acd683ccb34e33376189f6b7ed2f5eb441dc61d592cad2592256dfea51296330752181b9403
src/isa/aarch64/lower.isle 64a725771537f69c445f44c728e04bffd8a715d6a4d87a5a2bf2e89714ee290b7497c5ca8b335bdddd775f6734be03318ff9aa67e2e4068949ebae06b0902b3f
src/prelude.isle d3d2a6a42fb778231a4cdca30995324e1293a9ca8073c5a27a501535759eb51f84a6718322a93dfba4b66ee4f0c9afce7dcec0428516ef0c5bc96e8c8b76925d
src/isa/aarch64/inst.isle cec03d88680e8da01424eecc05ef73a48e4055d29fe841fceaa3e6ea4e7cb9abb887401bb5acb2e058c9fc993188640990b699e88272d62e243781b231cdfb0d
src/isa/aarch64/lower.isle e1ae53adc953ad395feeecd8edc8bcfd288491a4e4a71510e5f06e221f767518c6e060ff0d795c7c2510b7d898cc8b9bc0313906412e0176605c33427926f828
Loading