Skip to content

Commit

Permalink
pulley: Refactor conditional branches and table codegen
Browse files Browse the repository at this point in the history 
This commit first fixed an issue with table access codegen to disable
spectre mitigations on Pulley targets like how spectre is disabled for
memory accesses as well. This unblocked many tests related to tables
which then led to a different error about a `trapnz` with an 8-bit value
not being supported.

In fixing `trapnz` with 8-bit values this PR went ahead and did a
general-purpose refactoring for how conditional branches are managed.
Previously conditional traps and conditional branches had some
duplicated logic and the goal was to unify everything. There is now a
single `Cond` which represents the condition of a conditional jump which
is used uniformly for all locations such as `select`, `brif`, and
`trap[n]z`. This new type represents all the sorts of conditional
branches that can be done in Pulley, for example integer comparisons and
whether or not a register is zero. This `Cond` type has various helpers
for printing it, inverting it, collecting operands, emission, etc.

The end result is that it's a bit wordy to work with `Cond` right now
due to the size of the variants but all locations working with
conditional traps are deduplicated and now it's just repetitive logic
rather than duplicated logic.

Putting all of this together gets a large batch of spec tests working.

I'll note that this does remove a feature where `trapnz` was turned into
nothing or an unconditional trap if the argument was a constant, but
that feels like an optimization perhaps best left for the middle-end
rather than doing it in the backend.

cc bytecodealliance#9783
  • Loading branch information
@alexcrichton
alexcrichton committed Dec 11, 2024
1 parent ef14923 commit fc2a35b
Show file tree
Hide file tree
Showing 14 changed files with 388 additions and 607 deletions.
84 changes: 42 additions & 42 deletions cranelift/codegen/src/isa/pulley_shared/inst.isle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@

;;;; Actual Instructions ;;;;

;; Trap if `src1 cond src2`.
(TrapIf (cond IntCC) (size OperandSize) (src1 XReg) (src2 XReg) (code TrapCode))
;; Trap if `cond` is true.
(TrapIf (cond Cond) (code TrapCode))

;; Nothing.
(Nop)
Expand Down Expand Up @@ -60,16 +60,8 @@
;; Unconditional jumps.
(Jump (label MachLabel))

;; Jump to `then` if `c` is nonzero, otherwise to `else`.
(BrIf32 (c XReg) (taken MachLabel) (not_taken MachLabel))

;; Compare-and-branch macro ops.
(BrIfXeq32 (src1 XReg) (src2 XReg) (taken MachLabel) (not_taken MachLabel))
(BrIfXneq32 (src1 XReg) (src2 XReg) (taken MachLabel) (not_taken MachLabel))
(BrIfXslt32 (src1 XReg) (src2 XReg) (taken MachLabel) (not_taken MachLabel))
(BrIfXslteq32 (src1 XReg) (src2 XReg) (taken MachLabel) (not_taken MachLabel))
(BrIfXult32 (src1 XReg) (src2 XReg) (taken MachLabel) (not_taken MachLabel))
(BrIfXulteq32 (src1 XReg) (src2 XReg) (taken MachLabel) (not_taken MachLabel))
;; Jump to `then` if `c` is true, otherwise to `else`.
(BrIf (cond Cond) (taken MachLabel) (not_taken MachLabel))

;; Load the memory address referenced by `mem` into `dst`.
(LoadAddr (dst WritableXReg) (mem Amode))
Expand All @@ -95,6 +87,38 @@
)
)

;; Helper type on conditional branches and traps to represent what the
;; condition that is being performed is.
;;
;; Used in `BrIf` and `TrapIf` above for example.
(type Cond
(enum
;; True if `reg` contains a nonzero value in the low 32-bits.
(If32 (reg XReg))
;; True if `reg` contains a zero in the low 32-bits.
(IfNot32 (reg XReg))

;; Conditionals for comparing the low 32-bits of two registers.
(IfXeq32 (src1 XReg) (src2 XReg))
(IfXneq32 (src1 XReg) (src2 XReg))
(IfXslt32 (src1 XReg) (src2 XReg))
(IfXslteq32 (src1 XReg) (src2 XReg))
(IfXult32 (src1 XReg) (src2 XReg))
(IfXulteq32 (src1 XReg) (src2 XReg))

;; Conditionals for comparing two 64-bit registers.
(IfXeq64 (src1 XReg) (src2 XReg))
(IfXneq64 (src1 XReg) (src2 XReg))
(IfXslt64 (src1 XReg) (src2 XReg))
(IfXslteq64 (src1 XReg) (src2 XReg))
(IfXult64 (src1 XReg) (src2 XReg))
(IfXulteq64 (src1 XReg) (src2 XReg))
)
)

(decl cond_invert (Cond) Cond)
(extern constructor cond_invert cond_invert)

(decl raw_inst_to_inst (RawInst) MInst)
(rule (raw_inst_to_inst inst) (MInst.Raw inst))
(convert RawInst MInst raw_inst_to_inst)
Expand Down Expand Up @@ -349,9 +373,9 @@

;;;; Instruction Constructors ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(decl pulley_trap_if (IntCC OperandSize XReg XReg TrapCode) SideEffectNoResult)
(rule (pulley_trap_if cond size src1 src2 code)
(SideEffectNoResult.Inst (MInst.TrapIf cond size src1 src2 code)))
(decl pulley_trap_if (Cond TrapCode) SideEffectNoResult)
(rule (pulley_trap_if cond code)
(SideEffectNoResult.Inst (MInst.TrapIf cond code)))

(decl sp_reg () XReg)
(extern constructor sp_reg sp_reg)
Expand All @@ -372,33 +396,9 @@
(rule (pulley_jump label)
(SideEffectNoResult.Inst (MInst.Jump label)))

(decl pulley_br_if32 (XReg MachLabel MachLabel) SideEffectNoResult)
(rule (pulley_br_if32 c taken not_taken)
(SideEffectNoResult.Inst (MInst.BrIf32 c taken not_taken)))

(decl pulley_br_if_xeq32 (XReg XReg MachLabel MachLabel) SideEffectNoResult)
(rule (pulley_br_if_xeq32 a b taken not_taken)
(SideEffectNoResult.Inst (MInst.BrIfXeq32 a b taken not_taken)))

(decl pulley_br_if_xneq32 (XReg XReg MachLabel MachLabel) SideEffectNoResult)
(rule (pulley_br_if_xneq32 a b taken not_taken)
(SideEffectNoResult.Inst (MInst.BrIfXneq32 a b taken not_taken)))

(decl pulley_br_if_xslt32 (XReg XReg MachLabel MachLabel) SideEffectNoResult)
(rule (pulley_br_if_xslt32 a b taken not_taken)
(SideEffectNoResult.Inst (MInst.BrIfXslt32 a b taken not_taken)))

(decl pulley_br_if_xslteq32 (XReg XReg MachLabel MachLabel) SideEffectNoResult)
(rule (pulley_br_if_xslteq32 a b taken not_taken)
(SideEffectNoResult.Inst (MInst.BrIfXslteq32 a b taken not_taken)))

(decl pulley_br_if_xult32 (XReg XReg MachLabel MachLabel) SideEffectNoResult)
(rule (pulley_br_if_xult32 a b taken not_taken)
(SideEffectNoResult.Inst (MInst.BrIfXult32 a b taken not_taken)))

(decl pulley_br_if_xulteq32 (XReg XReg MachLabel MachLabel) SideEffectNoResult)
(rule (pulley_br_if_xulteq32 a b taken not_taken)
(SideEffectNoResult.Inst (MInst.BrIfXulteq32 a b taken not_taken)))
(decl pulley_br_if (Cond MachLabel MachLabel) SideEffectNoResult)
(rule (pulley_br_if cond taken not_taken)
(SideEffectNoResult.Inst (MInst.BrIf cond taken not_taken)))

(decl pulley_xload (Amode Type MemFlags ExtKind) XReg)
(rule (pulley_xload amode ty flags ext)
Expand Down
144 changes: 144 additions & 0 deletions cranelift/codegen/src/isa/pulley_shared/inst/args.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
use super::*;
use crate::machinst::abi::StackAMode;
use pulley_interpreter::encode;
use pulley_interpreter::regs::Reg as _;
use std::fmt;

/// A macro for defining a newtype of `Reg` that enforces some invariant about
/// the wrapped `Reg` (such as that it is of a particular register class).
Expand Down Expand Up @@ -229,3 +231,145 @@ pub enum OperandSize {
/// 64 bits.
Size64,
}

pub use crate::isa::pulley_shared::lower::isle::generated_code::Cond;

impl Cond {
/// Collect register operands within `collector` for register allocation.
pub fn get_operands(&mut self, collector: &mut impl OperandVisitor) {
match self {
Cond::If32 { reg } | Cond::IfNot32 { reg } => collector.reg_use(reg),

Cond::IfXeq32 { src1, src2 }
| Cond::IfXneq32 { src1, src2 }
| Cond::IfXslt32 { src1, src2 }
| Cond::IfXslteq32 { src1, src2 }
| Cond::IfXult32 { src1, src2 }
| Cond::IfXulteq32 { src1, src2 }
| Cond::IfXeq64 { src1, src2 }
| Cond::IfXneq64 { src1, src2 }
| Cond::IfXslt64 { src1, src2 }
| Cond::IfXslteq64 { src1, src2 }
| Cond::IfXult64 { src1, src2 }
| Cond::IfXulteq64 { src1, src2 } => {
collector.reg_use(src1);
collector.reg_use(src2);
}
}
}

/// Encode this condition as a branch into `sink`.
///
/// Note that the offset encoded to jump by is filled in as 0 and it's
/// assumed `MachBuffer` will come back and clean it up.
pub fn encode(&self, sink: &mut impl Extend<u8>) {
match self {
Cond::If32 { reg } => encode::br_if32(sink, reg, 0),
Cond::IfNot32 { reg } => encode::br_if_not32(sink, reg, 0),
Cond::IfXeq32 { src1, src2 } => encode::br_if_xeq32(sink, src1, src2, 0),
Cond::IfXneq32 { src1, src2 } => encode::br_if_xneq32(sink, src1, src2, 0),
Cond::IfXslt32 { src1, src2 } => encode::br_if_xslt32(sink, src1, src2, 0),
Cond::IfXslteq32 { src1, src2 } => encode::br_if_xslteq32(sink, src1, src2, 0),
Cond::IfXult32 { src1, src2 } => encode::br_if_xult32(sink, src1, src2, 0),
Cond::IfXulteq32 { src1, src2 } => encode::br_if_xulteq32(sink, src1, src2, 0),
Cond::IfXeq64 { src1, src2 } => encode::br_if_xeq64(sink, src1, src2, 0),
Cond::IfXneq64 { src1, src2 } => encode::br_if_xneq64(sink, src1, src2, 0),
Cond::IfXslt64 { src1, src2 } => encode::br_if_xslt64(sink, src1, src2, 0),
Cond::IfXslteq64 { src1, src2 } => encode::br_if_xslteq64(sink, src1, src2, 0),
Cond::IfXult64 { src1, src2 } => encode::br_if_xult64(sink, src1, src2, 0),
Cond::IfXulteq64 { src1, src2 } => encode::br_if_xulteq64(sink, src1, src2, 0),
}
}

/// Inverts this conditional.
pub fn invert(&self) -> Cond {
match *self {
Cond::If32 { reg } => Cond::IfNot32 { reg },
Cond::IfNot32 { reg } => Cond::If32 { reg },
Cond::IfXeq32 { src1, src2 } => Cond::IfXneq32 { src1, src2 },
Cond::IfXneq32 { src1, src2 } => Cond::IfXeq32 { src1, src2 },
Cond::IfXeq64 { src1, src2 } => Cond::IfXneq64 { src1, src2 },
Cond::IfXneq64 { src1, src2 } => Cond::IfXeq64 { src1, src2 },

// Note that for below the condition changes but the operands are
// also swapped.
Cond::IfXslt32 { src1, src2 } => Cond::IfXslteq32 {
src1: src2,
src2: src1,
},
Cond::IfXslteq32 { src1, src2 } => Cond::IfXslt32 {
src1: src2,
src2: src1,
},
Cond::IfXult32 { src1, src2 } => Cond::IfXulteq32 {
src1: src2,
src2: src1,
},
Cond::IfXulteq32 { src1, src2 } => Cond::IfXult32 {
src1: src2,
src2: src1,
},
Cond::IfXslt64 { src1, src2 } => Cond::IfXslteq64 {
src1: src2,
src2: src1,
},
Cond::IfXslteq64 { src1, src2 } => Cond::IfXslt64 {
src1: src2,
src2: src1,
},
Cond::IfXult64 { src1, src2 } => Cond::IfXulteq64 {
src1: src2,
src2: src1,
},
Cond::IfXulteq64 { src1, src2 } => Cond::IfXult64 {
src1: src2,
src2: src1,
},
}
}
}

impl fmt::Display for Cond {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Cond::If32 { reg } => write!(f, "if32 {}", reg_name(**reg)),
Cond::IfNot32 { reg } => write!(f, "if_not32 {}", reg_name(**reg)),
Cond::IfXeq32 { src1, src2 } => {
write!(f, "if_xeq32 {}, {}", reg_name(**src1), reg_name(**src2))
}
Cond::IfXneq32 { src1, src2 } => {
write!(f, "if_xneq32 {}, {}", reg_name(**src1), reg_name(**src2))
}
Cond::IfXslt32 { src1, src2 } => {
write!(f, "if_xslt32 {}, {}", reg_name(**src1), reg_name(**src2))
}
Cond::IfXslteq32 { src1, src2 } => {
write!(f, "if_xslteq32 {}, {}", reg_name(**src1), reg_name(**src2))
}
Cond::IfXult32 { src1, src2 } => {
write!(f, "if_xult32 {}, {}", reg_name(**src1), reg_name(**src2))
}
Cond::IfXulteq32 { src1, src2 } => {
write!(f, "if_xulteq32 {}, {}", reg_name(**src1), reg_name(**src2))
}
Cond::IfXeq64 { src1, src2 } => {
write!(f, "if_xeq64 {}, {}", reg_name(**src1), reg_name(**src2))
}
Cond::IfXneq64 { src1, src2 } => {
write!(f, "if_xneq64 {}, {}", reg_name(**src1), reg_name(**src2))
}
Cond::IfXslt64 { src1, src2 } => {
write!(f, "if_xslt64 {}, {}", reg_name(**src1), reg_name(**src2))
}
Cond::IfXslteq64 { src1, src2 } => {
write!(f, "if_xslteq64 {}, {}", reg_name(**src1), reg_name(**src2))
}
Cond::IfXult64 { src1, src2 } => {
write!(f, "if_xult64 {}, {}", reg_name(**src1), reg_name(**src2))
}
Cond::IfXulteq64 { src1, src2 } => {
write!(f, "if_xulteq64 {}, {}", reg_name(**src1), reg_name(**src2))
}
}
}
}
Loading

0 comments on commit fc2a35b

Please sign in to comment.