From b948de16932512e556b48e472e274d43cca31896 Mon Sep 17 00:00:00 2001 From: Jakob Stoklund Olesen Date: Wed, 18 Oct 2017 14:03:46 -0700 Subject: [PATCH] Add a verifier pass for CPU flags. Only one CPU flags value can be live at a time, and some instructions clobber the flags. --- .../filetests/isa/riscv/verify-encoding.cton | 2 +- cranelift/filetests/verifier/flags.cton | 76 +++++++++ lib/cretonne/src/entity/sparse.rs | 5 + lib/cretonne/src/ir/dfg.rs | 7 +- lib/cretonne/src/ir/types.rs | 8 + lib/cretonne/src/verifier/flags.rs | 151 ++++++++++++++++++ lib/cretonne/src/verifier/mod.rs | 17 +- 7 files changed, 259 insertions(+), 7 deletions(-) create mode 100644 cranelift/filetests/verifier/flags.cton create mode 100644 lib/cretonne/src/verifier/flags.rs diff --git a/cranelift/filetests/isa/riscv/verify-encoding.cton b/cranelift/filetests/isa/riscv/verify-encoding.cton index 942179ba23..725b9e5744 100644 --- a/cranelift/filetests/isa/riscv/verify-encoding.cton +++ b/cranelift/filetests/isa/riscv/verify-encoding.cton @@ -16,6 +16,6 @@ function %RV32I(i32 link [%x1]) -> i32 link [%x1] { ebb0(v9999: i32): v1 = iconst.i32 1 v2 = iconst.i32 2 - [R#0,-] v3 = iadd v1, v2 ; error: Instruction encoding R#00 doesn't match any possibilities + [R#0,-] v3 = iadd v1, v2 ; error: encoding R#00 should be R#0c return v9999 } diff --git a/cranelift/filetests/verifier/flags.cton b/cranelift/filetests/verifier/flags.cton new file mode 100644 index 0000000000..dff01244e2 --- /dev/null +++ b/cranelift/filetests/verifier/flags.cton @@ -0,0 +1,76 @@ +test verifier +isa intel + +; Simple, correct use of CPU flags. +function %simple(i32) -> i32 { + ebb0(v0: i32): + [Op1rcmp#39] v1 = ifcmp v0, v0 + [Op2seti_abcd#490] v2 = trueif ugt v1 + [Op2urm_abcd#4b6] v3 = bint.i32 v2 + [Op1ret#c3] return v3 +} + +; Overlapping flag values of different types. +function %overlap(i32, f32) -> i32 { + ebb0(v0: i32, v1: f32): + [Op1rcmp#39] v2 = ifcmp v0, v0 + [Op2fcmp#42e] v3 = ffcmp v1, v1 + [Op2setf_abcd#490] v4 = trueff gt v3 ; error: conflicting live CPU flags: v2 and v3 + [Op2seti_abcd#490] v5 = trueif ugt v2 + [Op1rr#21] v6 = band v4, v5 + [Op2urm_abcd#4b6] v7 = bint.i32 v6 + [Op1ret#c3] return v7 +} + +; CPU flags clobbered by arithmetic. +function %clobbered(i32) -> i32 { + ebb0(v0: i32): + [Op1rcmp#39] v1 = ifcmp v0, v0 + [Op1rr#01] v2 = iadd v0, v0 ; error: encoding clobbers live CPU flags in v1 + [Op2seti_abcd#490] v3 = trueif ugt v1 + [Op2urm_abcd#4b6] v4 = bint.i32 v3 + [Op1ret#c3] return v4 +} + +; CPU flags not clobbered by load. +function %live_across_load(i32) -> i32 { + ebb0(v0: i32): + [Op1rcmp#39] v1 = ifcmp v0, v0 + [Op1ld#8b] v2 = load.i32 v0 + [Op2seti_abcd#490] v3 = trueif ugt v1 + [Op2urm_abcd#4b6] v4 = bint.i32 v3 + [Op1ret#c3] return v4 +} + +; Correct use of CPU flags across EBB. +function %live_across_ebb(i32) -> i32 { + ebb0(v0: i32): + [Op1rcmp#39] v1 = ifcmp v0, v0 + [Op1jmpb#eb] jump ebb1 + ebb1: + [Op2seti_abcd#490] v2 = trueif ugt v1 + [Op2urm_abcd#4b6] v3 = bint.i32 v2 + [Op1ret#c3] return v3 +} + +function %live_across_ebb_backwards(i32) -> i32 { + ebb0(v0: i32): + [Op1jmpb#eb] jump ebb2 + ebb1: + [Op2seti_abcd#490] v2 = trueif ugt v1 + [Op2urm_abcd#4b6] v3 = bint.i32 v2 + [Op1ret#c3] return v3 + ebb2: + [Op1rcmp#39] v1 = ifcmp v0, v0 + [Op1jmpb#eb] jump ebb1 +} + +; Flags live into loop. +function %live_into_loop(i32) -> i32 { + ebb0(v0: i32): + [Op1rcmp#39] v1 = ifcmp v0, v0 + [Op1jmpb#eb] jump ebb1 + ebb1: + [Op2seti_abcd#490] v2 = trueif ugt v1 + [Op1jmpb#eb] jump ebb1 +} \ No newline at end of file diff --git a/lib/cretonne/src/entity/sparse.rs b/lib/cretonne/src/entity/sparse.rs index 1476092121..26be799afd 100644 --- a/lib/cretonne/src/entity/sparse.rs +++ b/lib/cretonne/src/entity/sparse.rs @@ -176,6 +176,11 @@ where None } + /// Remove the last value from the map. + pub fn pop(&mut self) -> Option { + self.dense.pop() + } + /// Get an iterator over the values in the map. /// /// The iteration order is entirely determined by the preceding sequence of `insert` and diff --git a/lib/cretonne/src/ir/dfg.rs b/lib/cretonne/src/ir/dfg.rs index dbb643e7f3..b3d13c261d 100644 --- a/lib/cretonne/src/ir/dfg.rs +++ b/lib/cretonne/src/ir/dfg.rs @@ -4,7 +4,7 @@ use entity::{PrimaryMap, EntityMap}; use isa::TargetIsa; use ir::builder::{InsertBuilder, ReplaceBuilder}; use ir::extfunc::ExtFuncData; -use ir::instructions::{InstructionData, CallInfo}; +use ir::instructions::{InstructionData, CallInfo, BranchInfo}; use ir::layout::{Cursor, LayoutCursorInserter}; use ir::types; use ir::{Ebb, Inst, Value, Type, SigRef, Signature, FuncRef, ValueList, ValueListPool}; @@ -613,6 +613,11 @@ impl DataFlowGraph { } } + /// Check if `inst` is a branch. + pub fn analyze_branch(&self, inst: Inst) -> BranchInfo { + self.insts[inst].analyze_branch(&self.value_lists) + } + /// Compute the type of an instruction result from opcode constraints and call signatures. /// /// This computes the same sequence of result types that `make_inst_results()` above would diff --git a/lib/cretonne/src/ir/types.rs b/lib/cretonne/src/ir/types.rs index 1a20f9e511..e0cdcd02ce 100644 --- a/lib/cretonne/src/ir/types.rs +++ b/lib/cretonne/src/ir/types.rs @@ -200,6 +200,14 @@ impl Type { } } + /// Is this a CPU flags type? + pub fn is_flags(self) -> bool { + match self { + IFLAGS | FFLAGS => true, + _ => false, + } + } + /// Get log_2 of the number of lanes in this SIMD vector type. /// /// All SIMD types have a lane count that is a power of two and no larger than 256, so this diff --git a/lib/cretonne/src/verifier/flags.rs b/lib/cretonne/src/verifier/flags.rs new file mode 100644 index 0000000000..c2f3b5340a --- /dev/null +++ b/lib/cretonne/src/verifier/flags.rs @@ -0,0 +1,151 @@ +//! Verify CPU flags values. + +use entity::{EntityMap, SparseSet}; +use flowgraph::ControlFlowGraph; +use ir::instructions::BranchInfo; +use ir; +use isa; +use packed_option::PackedOption; +use std::result; +use verifier::{Result, Error}; + +/// Verify that CPU flags are used correctly. +/// +/// The value types `iflags` and `fflags` represent CPU flags which usually live in a +/// special-purpose register, so they can't be used as freely as other value types that can live in +/// any register. +/// +/// We verify the following conditions: +/// +/// - At most one flags value can be live at a time. +/// - A flags value can not be live across an instruction that clobbers the flags. +/// +/// +pub fn verify_flags( + func: &ir::Function, + cfg: &ControlFlowGraph, + isa: Option<&isa::TargetIsa>, +) -> Result { + let mut verifier = FlagsVerifier { + func, + cfg, + encinfo: isa.map(|isa| isa.encoding_info()), + livein: EntityMap::new(), + }; + verifier.check() +} + +struct FlagsVerifier<'a> { + func: &'a ir::Function, + cfg: &'a ControlFlowGraph, + encinfo: Option, + + /// The single live-in flags value (if any) for each EBB. + livein: EntityMap>, +} + +impl<'a> FlagsVerifier<'a> { + fn check(&mut self) -> Result { + // List of EBBs that need to be processed. EBBs may be re-added to this list when we detect + // that one of their successor blocks needs a live-in flags value. + let mut worklist = SparseSet::new(); + for ebb in self.func.layout.ebbs() { + worklist.insert(ebb); + } + + while let Some(ebb) = worklist.pop() { + if let Some(value) = self.visit_ebb(ebb)? { + // The EBB has live-in flags. Check if the value changed. + match self.livein[ebb].expand() { + // Revisit any predecessor blocks the first time we see a live-in for `ebb`. + None => { + self.livein[ebb] = value.into(); + for &(pred, _) in self.cfg.get_predecessors(ebb) { + worklist.insert(pred); + } + } + Some(old) if old != value => { + return err!(ebb, "conflicting live-in CPU flags: {} and {}", old, value); + } + x => assert_eq!(x, Some(value)), + } + } else { + // Existing live-in flags should never be able to disappear. + assert_eq!(self.livein[ebb].expand(), None); + } + } + + Ok(()) + } + + /// Check flags usage in `ebb` and return the live-in flags value, if any. + fn visit_ebb(&self, ebb: ir::Ebb) -> result::Result, Error> { + // The single currently live flags value. + let mut live_val = None; + + // Visit instructions backwards so we can track liveness accurately. + for inst in self.func.layout.ebb_insts(ebb).rev() { + // Check if `inst` interferes with existing live flags. + if let Some(live) = live_val { + for &res in self.func.dfg.inst_results(inst) { + if res == live { + // We've reached the def of `live_flags`, so it is no longer live above. + live_val = None; + } else if self.func.dfg.value_type(res).is_flags() { + return err!(inst, "{} clobbers live CPU flags in {}", res, live); + } + } + + // Does the instruction have an encoding that clobbers the CPU flags? + if self.encinfo + .as_ref() + .and_then(|ei| ei.operand_constraints(self.func.encodings[inst])) + .map(|c| c.clobbers_flags) + .unwrap_or(false) && live_val.is_some() + { + return err!(inst, "encoding clobbers live CPU flags in {}", live); + } + } + + // Now look for live ranges of CPU flags that end here. + for &arg in self.func.dfg.inst_args(inst) { + if self.func.dfg.value_type(arg).is_flags() { + merge(&mut live_val, arg, inst)?; + } + } + + // Include live-in flags to successor EBBs. + match self.func.dfg.analyze_branch(inst) { + BranchInfo::NotABranch => {} + BranchInfo::SingleDest(dest, _) => { + if let Some(val) = self.livein[dest].expand() { + merge(&mut live_val, val, inst)?; + } + } + BranchInfo::Table(jt) => { + for (_, dest) in self.func.jump_tables[jt].entries() { + if let Some(val) = self.livein[dest].expand() { + merge(&mut live_val, val, inst)?; + } + } + } + } + } + + // Return the required live-in flags value. + Ok(live_val) + } +} + +// Merge live flags values, or return an error on conflicting values. +fn merge(a: &mut Option, b: ir::Value, inst: ir::Inst) -> Result { + if let Some(va) = *a { + if b != va { + return err!(inst, "conflicting live CPU flags: {} and {}", va, b); + } + } else { + *a = Some(b); + } + + Ok(()) +} diff --git a/lib/cretonne/src/verifier/mod.rs b/lib/cretonne/src/verifier/mod.rs index f143531148..0b9b2db142 100644 --- a/lib/cretonne/src/verifier/mod.rs +++ b/lib/cretonne/src/verifier/mod.rs @@ -59,19 +59,20 @@ use dbg::DisplayList; use dominator_tree::DominatorTree; use entity::SparseSet; use flowgraph::ControlFlowGraph; -use ir; use ir::entities::AnyEntity; use ir::instructions::{InstructionFormat, BranchInfo, ResolvedConstraint, CallInfo}; use ir::{types, Function, ValueDef, Ebb, Inst, SigRef, FuncRef, ValueList, JumpTable, StackSlot, StackSlotKind, GlobalVar, Value, Type, Opcode, ValueLoc, ArgumentLoc}; +use ir; use isa::TargetIsa; +use iterators::IteratorExtras; +use self::flags::verify_flags; use settings::{Flags, FlagsOrIsa}; +use std::cmp::Ordering; +use std::collections::BTreeSet; use std::error as std_error; use std::fmt::{self, Display, Formatter, Write}; use std::result; -use std::collections::BTreeSet; -use std::cmp::Ordering; -use iterators::IteratorExtras; pub use self::cssa::verify_cssa; pub use self::liveness::verify_liveness; @@ -95,6 +96,7 @@ macro_rules! err { } mod cssa; +mod flags; mod liveness; mod locations; @@ -980,6 +982,7 @@ impl<'a> Verifier<'a> { if !has_valid_encoding { let mut possible_encodings = String::new(); + let mut multiple_encodings = false; for enc in isa.legal_encodings( &self.func.dfg, @@ -989,6 +992,7 @@ impl<'a> Verifier<'a> { { if possible_encodings.len() != 0 { possible_encodings.push_str(", "); + multiple_encodings = true; } possible_encodings .write_fmt(format_args!("{}", isa.encoding_info().display(enc))) @@ -997,8 +1001,9 @@ impl<'a> Verifier<'a> { return err!( inst, - "Instruction encoding {} doesn't match any possibilities: [{}]", + "encoding {} should be {}{}", isa.encoding_info().display(encoding), + if multiple_encodings { "one of: " } else { "" }, possible_encodings ); } @@ -1085,6 +1090,8 @@ impl<'a> Verifier<'a> { self.verify_return_at_end()?; } + verify_flags(self.func, &self.cfg, self.isa)?; + Ok(()) } }