From b58a197d33f044193c3d608010f5e6ec394ac07e Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Tue, 24 Jan 2023 14:37:16 -0800 Subject: [PATCH] cranelift: Add a conditional branch instruction with two targets (#5446) Add a conditional branch instruction with two targets: brif. This instruction will eventually replace brz and brnz, as it encompasses the behavior of both. This PR also changes the InstructionData layout for instruction formats that hold BlockCall values, taking the same approach we use for Value arguments. This allows branch_destination to return a slice to the BlockCall values held in the instruction, rather than requiring that we pattern match on InstructionData to fetch the then/else blocks. Function generation for fuzzing has been updated to generate uses of brif, and I've run the cranelift-fuzzgen target locally for hours without triggering any new failures. --- cranelift/codegen/meta/src/cdsl/formats.rs | 14 +- cranelift/codegen/meta/src/gen_inst.rs | 162 ++++++++++++++- cranelift/codegen/meta/src/shared/entities.rs | 20 ++ cranelift/codegen/meta/src/shared/formats.rs | 10 +- .../codegen/meta/src/shared/instructions.rs | 21 ++ cranelift/codegen/src/dominator_tree.rs | 4 + cranelift/codegen/src/flowgraph.rs | 4 + cranelift/codegen/src/inst_predicates.rs | 34 ++- cranelift/codegen/src/ir/dfg.rs | 6 +- cranelift/codegen/src/ir/function.rs | 44 ++-- cranelift/codegen/src/ir/instructions.rs | 44 ++-- cranelift/codegen/src/isa/aarch64/lower.isle | 46 +++++ .../codegen/src/isa/aarch64/lower/isle.rs | 4 +- cranelift/codegen/src/isa/riscv64/inst.isle | 24 +++ .../codegen/src/isa/riscv64/lower/isle.rs | 4 +- cranelift/codegen/src/isa/s390x/lower.isle | 11 + cranelift/codegen/src/isa/s390x/lower/isle.rs | 4 +- cranelift/codegen/src/isa/x64/lower.isle | 19 ++ cranelift/codegen/src/isa/x64/lower/isle.rs | 2 +- cranelift/codegen/src/isle_prelude.rs | 11 + cranelift/codegen/src/machinst/isle.rs | 3 +- cranelift/codegen/src/machinst/lower.rs | 37 +++- cranelift/codegen/src/prelude.isle | 1 + cranelift/codegen/src/remove_constant_phis.rs | 25 ++- cranelift/codegen/src/verifier/mod.rs | 28 +++ cranelift/codegen/src/write.rs | 10 + .../filetests/filetests/isa/x64/branches.clif | 62 ++++++ .../filetests/filetests/parser/branch.clif | 21 ++ .../filetests/filetests/runtests/brif.clif | 195 ++++++++++++++++++ .../filetests/verifier/type_check.clif | 16 ++ cranelift/frontend/src/frontend.rs | 93 ++++----- cranelift/frontend/src/ssa.rs | 16 +- cranelift/fuzzgen/src/function_generator.rs | 23 ++- cranelift/interpreter/src/step.rs | 64 ++++-- cranelift/reader/src/parser.rs | 20 ++ 35 files changed, 943 insertions(+), 159 deletions(-) create mode 100644 cranelift/filetests/filetests/runtests/brif.clif diff --git a/cranelift/codegen/meta/src/cdsl/formats.rs b/cranelift/codegen/meta/src/cdsl/formats.rs index 876fb7702f..10d804a98a 100644 --- a/cranelift/codegen/meta/src/cdsl/formats.rs +++ b/cranelift/codegen/meta/src/cdsl/formats.rs @@ -38,6 +38,8 @@ pub(crate) struct InstructionFormat { pub imm_fields: Vec, + pub num_block_operands: usize, + /// Index of the value input operand that is used to infer the controlling type variable. By /// default, this is `0`, the first `value` operand. The index is relative to the values only, /// ignoring immediate operands. @@ -49,6 +51,7 @@ pub(crate) struct InstructionFormat { pub(crate) struct FormatStructure { pub num_value_operands: usize, pub has_value_list: bool, + pub num_block_operands: usize, /// Tuples of (Rust field name / Rust type) for each immediate field. pub imm_field_names: Vec<(&'static str, &'static str)>, } @@ -62,8 +65,8 @@ impl fmt::Display for InstructionFormat { .collect::>() .join(", "); fmt.write_fmt(format_args!( - "{}(imms=({}), vals={})", - self.name, imm_args, self.num_value_operands + "{}(imms=({}), vals={}, blocks={})", + self.name, imm_args, self.num_value_operands, self.num_block_operands, ))?; Ok(()) } @@ -75,6 +78,7 @@ impl InstructionFormat { FormatStructure { num_value_operands: self.num_value_operands, has_value_list: self.has_value_list, + num_block_operands: self.num_block_operands, imm_field_names: self .imm_fields .iter() @@ -92,6 +96,7 @@ impl InstructionFormatBuilder { name, num_value_operands: 0, has_value_list: false, + num_block_operands: 0, imm_fields: Vec::new(), typevar_operand: None, }) @@ -107,6 +112,11 @@ impl InstructionFormatBuilder { self } + pub fn block(mut self) -> Self { + self.0.num_block_operands += 1; + self + } + pub fn imm(mut self, operand_kind: &OperandKind) -> Self { let field = FormatField { kind: operand_kind.clone(), diff --git a/cranelift/codegen/meta/src/gen_inst.rs b/cranelift/codegen/meta/src/gen_inst.rs index 85bee66abe..25b413c7d2 100644 --- a/cranelift/codegen/meta/src/gen_inst.rs +++ b/cranelift/codegen/meta/src/gen_inst.rs @@ -82,6 +82,18 @@ fn gen_instruction_data(formats: &[&InstructionFormat], fmt: &mut Formatter) { } else if format.num_value_operands > 0 { fmtln!(fmt, "args: [Value; {}],", format.num_value_operands); } + + match format.num_block_operands { + 0 => (), + 1 => fmt.line("destination: ir::BlockCall,"), + 2 => fmtln!( + fmt, + "blocks: [ir::BlockCall; {}],", + format.num_block_operands + ), + n => panic!("Too many block operands in instruction: {}", n), + } + for field in &format.imm_fields { fmtln!(fmt, "{}: {},", field.member, field.kind.rust_type); } @@ -248,6 +260,18 @@ fn gen_instruction_data_impl(formats: &[&InstructionFormat], fmt: &mut Formatter None }; + let blocks_eq = match format.num_block_operands { + 0 => None, + 1 => { + members.push("destination"); + Some("destination1 == destination2") + }, + _ => { + members.push("blocks"); + Some("blocks1.iter().zip(blocks2.iter()).all(|(a, b)| a.block(pool) == b.block(pool))") + } + }; + for field in &format.imm_fields { members.push(field.member); } @@ -263,6 +287,9 @@ fn gen_instruction_data_impl(formats: &[&InstructionFormat], fmt: &mut Formatter if let Some(args_eq) = args_eq { fmtln!(fmt, "&& {}", args_eq); } + if let Some(blocks_eq) = blocks_eq { + fmtln!(fmt, "&& {}", blocks_eq); + } }); fmtln!(fmt, "}"); } @@ -304,6 +331,18 @@ fn gen_instruction_data_impl(formats: &[&InstructionFormat], fmt: &mut Formatter ("&[]", "0") }; + let blocks = match format.num_block_operands { + 0 => None, + 1 => { + members.push("ref destination"); + Some(("std::slice::from_ref(destination)", "1")) + } + _ => { + members.push("ref blocks"); + Some(("blocks", "blocks.len()")) + } + }; + for field in &format.imm_fields { members.push(field.member); } @@ -323,6 +362,21 @@ fn gen_instruction_data_impl(formats: &[&InstructionFormat], fmt: &mut Formatter fmtln!(fmt, "::core::hash::Hash::hash(&arg, state);"); }); fmtln!(fmt, "}"); + + if let Some((blocks, len)) = blocks { + fmtln!(fmt, "::core::hash::Hash::hash(&{}, state);", len); + fmtln!(fmt, "for &block in {} {{", blocks); + fmt.indent(|fmt| { + fmtln!(fmt, "::core::hash::Hash::hash(&block.block(pool), state);"); + fmtln!(fmt, "for &arg in block.args_slice(pool) {"); + fmt.indent(|fmt| { + fmtln!(fmt, "let arg = mapper(arg);"); + fmtln!(fmt, "::core::hash::Hash::hash(&arg, state);"); + }); + fmtln!(fmt, "}"); + }); + fmtln!(fmt, "}"); + } }); fmtln!(fmt, "}"); } @@ -774,6 +828,19 @@ fn gen_member_inits(format: &InstructionFormat, fmt: &mut Formatter) { } fmtln!(fmt, "args: [{}],", args.join(", ")); } + + // Block operands + match format.num_block_operands { + 0 => (), + 1 => fmt.line("destination: block0"), + n => { + let mut blocks = Vec::new(); + for i in 0..n { + blocks.push(format!("block{}", i)); + } + fmtln!(fmt, "blocks: [{}],", blocks.join(", ")); + } + } } /// Emit a method for creating and inserting an instruction format. @@ -793,6 +860,9 @@ fn gen_format_constructor(format: &InstructionFormat, fmt: &mut Formatter) { args.push(format!("{}: {}", f.member, f.kind.rust_type)); } + // Then the block operands. + args.extend((0..format.num_block_operands).map(|i| format!("block{}: ir::BlockCall", i))); + // Then the value operands. if format.has_value_list { // Take all value arguments as a finished value list. The value lists @@ -1152,6 +1222,41 @@ fn gen_common_isle( fmt.empty_line(); } + // Generate all of the block arrays we need for `InstructionData` as well as + // the constructors and extractors for them. + fmt.line(";;;; Block Arrays ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;"); + fmt.empty_line(); + let block_array_arities: BTreeSet<_> = formats + .iter() + .filter(|f| f.num_block_operands > 1) + .map(|f| f.num_block_operands) + .collect(); + for n in block_array_arities { + fmtln!(fmt, ";; ISLE representation of `[BlockCall; {}]`.", n); + fmtln!(fmt, "(type BlockArray{} extern (enum))", n); + fmt.empty_line(); + + fmtln!( + fmt, + "(decl block_array_{0} ({1}) BlockArray{0})", + n, + (0..n).map(|_| "BlockCall").collect::>().join(" ") + ); + + fmtln!( + fmt, + "(extern constructor block_array_{0} pack_block_array_{0})", + n + ); + + fmtln!( + fmt, + "(extern extractor infallible block_array_{0} unpack_block_array_{0})", + n + ); + fmt.empty_line(); + } + // Generate the extern type declaration for `Opcode`. fmt.line(";;;; `Opcode` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;"); fmt.empty_line(); @@ -1187,6 +1292,13 @@ fn gen_common_isle( } else if format.num_value_operands > 1 { write!(&mut s, " (args ValueArray{})", format.num_value_operands).unwrap(); } + + match format.num_block_operands { + 0 => (), + 1 => write!(&mut s, " (destination BlockCall)").unwrap(), + n => write!(&mut s, " (blocks BlockArray{})", n).unwrap(), + } + for field in &format.imm_fields { write!( &mut s, @@ -1328,13 +1440,37 @@ fn gen_common_isle( let imm_operands: Vec<_> = inst .operands_in .iter() - .filter(|o| !o.is_value() && !o.is_varargs()) + .filter(|o| !o.is_value() && !o.is_varargs() && !o.kind.is_block()) .collect(); - assert_eq!(imm_operands.len(), inst.format.imm_fields.len()); + assert_eq!(imm_operands.len(), inst.format.imm_fields.len(),); for op in imm_operands { write!(&mut s, " {}", op.name).unwrap(); } + // Blocks. + let block_operands: Vec<_> = inst + .operands_in + .iter() + .filter(|o| o.kind.is_block()) + .collect(); + assert_eq!(block_operands.len(), inst.format.num_block_operands); + assert!(block_operands.len() <= 2); + + if !block_operands.is_empty() { + if block_operands.len() == 1 { + write!(&mut s, " {}", block_operands[0].name).unwrap(); + } else { + let blocks: Vec<_> = block_operands.iter().map(|o| o.name).collect(); + let blocks = blocks.join(" "); + write!( + &mut s, + " (block_array_{} {})", + inst.format.num_block_operands, blocks, + ) + .unwrap(); + } + } + s.push_str("))"); fmt.line(&s); }); @@ -1400,11 +1536,31 @@ fn gen_common_isle( .unwrap(); } + if inst.format.num_block_operands > 0 { + let blocks: Vec<_> = inst + .operands_in + .iter() + .filter(|o| o.kind.is_block()) + .map(|o| o.name) + .collect(); + if inst.format.num_block_operands == 1 { + write!(&mut s, " {}", blocks.first().unwrap(),).unwrap(); + } else { + write!( + &mut s, + " (block_array_{} {})", + inst.format.num_block_operands, + blocks.join(" ") + ) + .unwrap(); + } + } + // Immediates (non-value args). for o in inst .operands_in .iter() - .filter(|o| !o.is_value() && !o.is_varargs()) + .filter(|o| !o.is_value() && !o.is_varargs() && !o.kind.is_block()) { write!(&mut s, " {}", o.name).unwrap(); } diff --git a/cranelift/codegen/meta/src/shared/entities.rs b/cranelift/codegen/meta/src/shared/entities.rs index fdad737c7d..e713bc92ec 100644 --- a/cranelift/codegen/meta/src/shared/entities.rs +++ b/cranelift/codegen/meta/src/shared/entities.rs @@ -19,6 +19,14 @@ pub(crate) struct EntityRefs { /// This is primarily used in control flow instructions. pub(crate) label: OperandKind, + /// A reference to a basic block in the same function, with its arguments provided. + /// This is primarily used in control flow instructions. + pub(crate) block_then: OperandKind, + + /// A reference to a basic block in the same function, with its arguments provided. + /// This is primarily used in control flow instructions. + pub(crate) block_else: OperandKind, + /// A reference to a stack slot declared in the function preamble. pub(crate) stack_slot: OperandKind, @@ -61,6 +69,18 @@ impl EntityRefs { "a basic block in the same function.", ), + block_then: new( + "block_then", + "ir::BlockCall", + "a basic block in the same function, with its arguments provided.", + ), + + block_else: new( + "block_else", + "ir::BlockCall", + "a basic block in the same function, with its arguments provided.", + ), + stack_slot: new("stack_slot", "ir::StackSlot", "A stack slot"), dynamic_stack_slot: new( diff --git a/cranelift/codegen/meta/src/shared/formats.rs b/cranelift/codegen/meta/src/shared/formats.rs index c017836091..c7c70bcf23 100644 --- a/cranelift/codegen/meta/src/shared/formats.rs +++ b/cranelift/codegen/meta/src/shared/formats.rs @@ -8,6 +8,7 @@ pub(crate) struct Formats { pub(crate) binary: Rc, pub(crate) binary_imm8: Rc, pub(crate) binary_imm64: Rc, + pub(crate) brif: Rc, pub(crate) branch: Rc, pub(crate) branch_table: Rc, pub(crate) call: Rc, @@ -111,12 +112,11 @@ impl Formats { .value() .build(), - jump: Builder::new("Jump").imm(&entities.block_call).build(), + jump: Builder::new("Jump").block().build(), - branch: Builder::new("Branch") - .value() - .imm(&entities.block_call) - .build(), + branch: Builder::new("Branch").value().block().build(), + + brif: Builder::new("Brif").value().block().block().build(), branch_table: Builder::new("BranchTable") .value() diff --git a/cranelift/codegen/meta/src/shared/instructions.rs b/cranelift/codegen/meta/src/shared/instructions.rs index 6bc318b12e..a96f84861f 100755 --- a/cranelift/codegen/meta/src/shared/instructions.rs +++ b/cranelift/codegen/meta/src/shared/instructions.rs @@ -44,6 +44,27 @@ fn define_control_flow( TypeSetBuilder::new().ints(Interval::All).build(), ); + { + let c = &Operand::new("c", ScalarTruthy).with_doc("Controlling value to test"); + let block_then = &Operand::new("block_then", &entities.block_then).with_doc("Then block"); + let block_else = &Operand::new("block_else", &entities.block_else).with_doc("Else block"); + + ig.push( + Inst::new( + "brif", + r#" + Conditional branch when cond is non-zero. + + Take the ``then`` branch when ``c != 0``, and the ``else`` branch otherwise. + "#, + &formats.brif, + ) + .operands_in(vec![c, block_then, block_else]) + .is_terminator(true) + .is_branch(true), + ); + } + { let c = &Operand::new("c", ScalarTruthy).with_doc("Controlling value to test"); diff --git a/cranelift/codegen/src/dominator_tree.rs b/cranelift/codegen/src/dominator_tree.rs index 0d1200db7b..9c2e6e3627 100644 --- a/cranelift/codegen/src/dominator_tree.rs +++ b/cranelift/codegen/src/dominator_tree.rs @@ -356,6 +356,10 @@ impl DominatorTree { BranchInfo::SingleDest(succ) => { self.push_if_unseen(succ.block(&func.dfg.value_lists)) } + BranchInfo::Conditional(block_then, block_else) => { + self.push_if_unseen(block_then.block(&func.dfg.value_lists)); + self.push_if_unseen(block_else.block(&func.dfg.value_lists)); + } BranchInfo::Table(jt, dest) => { for succ in func.jump_tables[jt].iter() { self.push_if_unseen(*succ); diff --git a/cranelift/codegen/src/flowgraph.rs b/cranelift/codegen/src/flowgraph.rs index d5e61fca78..1c6c4fc563 100644 --- a/cranelift/codegen/src/flowgraph.rs +++ b/cranelift/codegen/src/flowgraph.rs @@ -125,6 +125,10 @@ impl ControlFlowGraph { BranchInfo::SingleDest(dest) => { self.add_edge(block, inst, dest.block(&func.dfg.value_lists)); } + BranchInfo::Conditional(block_then, block_else) => { + self.add_edge(block, inst, block_then.block(&func.dfg.value_lists)); + self.add_edge(block, inst, block_else.block(&func.dfg.value_lists)); + } BranchInfo::Table(jt, dest) => { self.add_edge(block, inst, dest); diff --git a/cranelift/codegen/src/inst_predicates.rs b/cranelift/codegen/src/inst_predicates.rs index 3c28899bd3..d18e4b80b4 100644 --- a/cranelift/codegen/src/inst_predicates.rs +++ b/cranelift/codegen/src/inst_predicates.rs @@ -176,25 +176,23 @@ pub(crate) fn visit_block_succs( mut visit: F, ) { for inst in f.layout.block_likely_branches(block) { - if f.dfg.insts[inst].opcode().is_branch() { - visit_branch_targets(f, inst, &mut visit); - } - } -} + match f.dfg.insts[inst].analyze_branch() { + BranchInfo::NotABranch => {} + BranchInfo::SingleDest(dest) => { + visit(inst, dest.block(&f.dfg.value_lists), false); + } + BranchInfo::Conditional(block_then, block_else) => { + visit(inst, block_then.block(&f.dfg.value_lists), false); + visit(inst, block_else.block(&f.dfg.value_lists), false); + } + BranchInfo::Table(table, dest) => { + // The default block is reached via a direct conditional branch, + // so it is not part of the table. + visit(inst, dest, false); -fn visit_branch_targets(f: &Function, inst: Inst, visit: &mut F) { - match f.dfg.insts[inst].analyze_branch() { - BranchInfo::NotABranch => {} - BranchInfo::SingleDest(dest) => { - visit(inst, dest.block(&f.dfg.value_lists), false); - } - BranchInfo::Table(table, dest) => { - // The default block is reached via a direct conditional branch, - // so it is not part of the table. - visit(inst, dest, false); - - for &dest in f.jump_tables[table].as_slice() { - visit(inst, dest, true); + for &dest in f.jump_tables[table].as_slice() { + visit(inst, dest, true); + } } } } diff --git a/cranelift/codegen/src/ir/dfg.rs b/cranelift/codegen/src/ir/dfg.rs index 74058938bb..49790bda1c 100644 --- a/cranelift/codegen/src/ir/dfg.rs +++ b/cranelift/codegen/src/ir/dfg.rs @@ -691,9 +691,10 @@ impl DataFlowGraph { self.inst_args_mut(inst)[i] = body(self, arg); } - for mut block in self.insts[inst].branch_destination().into_iter() { + for block_ix in 0..self.insts[inst].branch_destination().len() { // We aren't changing the size of the args list, so we won't need to write the branch // back to the instruction. + let mut block = self.insts[inst].branch_destination()[block_ix]; for i in 0..block.args_slice(&self.value_lists).len() { let arg = block.args_slice(&self.value_lists)[i]; block.args_slice_mut(&mut self.value_lists)[i] = body(self, arg); @@ -712,7 +713,8 @@ impl DataFlowGraph { *arg = values.next().unwrap(); } - for mut block in self.insts[inst].branch_destination().into_iter() { + for block_ix in 0..self.insts[inst].branch_destination().len() { + let mut block = self.insts[inst].branch_destination()[block_ix]; for arg in block.args_slice_mut(&mut self.value_lists) { *arg = values.next().unwrap(); } diff --git a/cranelift/codegen/src/ir/function.rs b/cranelift/codegen/src/ir/function.rs index bf1e2fa75b..5414831076 100644 --- a/cranelift/codegen/src/ir/function.rs +++ b/cranelift/codegen/src/ir/function.rs @@ -277,27 +277,41 @@ impl FunctionStencil { self.dfg.collect_debug_info(); } - /// Changes the destination of a jump or branch instruction. - /// Does nothing if called with a non-jump or non-branch instruction. - /// - /// Note that this method ignores multi-destination branches like `br_table`. - pub fn change_branch_destination(&mut self, inst: Inst, new_dest: Block) { - match self.dfg.insts[inst].branch_destination_mut() { - None => (), - Some(inst_dest) => inst_dest.set_block(new_dest, &mut self.dfg.value_lists), - } - } - /// Rewrite the branch destination to `new_dest` if the destination matches `old_dest`. /// Does nothing if called with a non-jump or non-branch instruction. - /// - /// Unlike [change_branch_destination](FunctionStencil::change_branch_destination), this method - /// rewrite the destinations of multi-destination branches like `br_table`. pub fn rewrite_branch_destination(&mut self, inst: Inst, old_dest: Block, new_dest: Block) { match self.dfg.analyze_branch(inst) { BranchInfo::SingleDest(dest) => { if dest.block(&self.dfg.value_lists) == old_dest { - self.change_branch_destination(inst, new_dest); + for block in self.dfg.insts[inst].branch_destination_mut() { + block.set_block(new_dest, &mut self.dfg.value_lists) + } + } + } + + BranchInfo::Conditional(block_then, block_else) => { + if block_then.block(&self.dfg.value_lists) == old_dest { + if let InstructionData::Brif { + blocks: [block_then, _], + .. + } = &mut self.dfg.insts[inst] + { + block_then.set_block(new_dest, &mut self.dfg.value_lists); + } else { + unreachable!(); + } + } + + if block_else.block(&self.dfg.value_lists) == old_dest { + if let InstructionData::Brif { + blocks: [_, block_else], + .. + } = &mut self.dfg.insts[inst] + { + block_else.set_block(new_dest, &mut self.dfg.value_lists); + } else { + unreachable!(); + } } } diff --git a/cranelift/codegen/src/ir/instructions.rs b/cranelift/codegen/src/ir/instructions.rs index 9301603525..e805aaca94 100644 --- a/cranelift/codegen/src/ir/instructions.rs +++ b/cranelift/codegen/src/ir/instructions.rs @@ -271,6 +271,10 @@ impl InstructionData { match *self { Self::Jump { destination, .. } => BranchInfo::SingleDest(destination), Self::Branch { destination, .. } => BranchInfo::SingleDest(destination), + Self::Brif { + blocks: [block_then, block_else], + .. + } => BranchInfo::Conditional(block_then, block_else), Self::BranchTable { table, destination, .. } => BranchInfo::Table(table, destination), @@ -281,27 +285,31 @@ impl InstructionData { } } - /// Get the single destination of this branch instruction, if it is a single destination - /// branch or jump. + /// Get the destinations of this instruction, if it's a branch. /// - /// Multi-destination branches like `br_table` return `None`. - pub fn branch_destination(&self) -> Option { - match *self { - Self::Jump { destination, .. } | Self::Branch { destination, .. } => Some(destination), - Self::BranchTable { .. } => None, + /// `br_table` returns the empty slice. + pub fn branch_destination(&self) -> &[BlockCall] { + match self { + Self::Jump { + ref destination, .. + } + | Self::Branch { + ref destination, .. + } => std::slice::from_ref(destination), + Self::Brif { blocks, .. } => blocks, + Self::BranchTable { .. } => &[], _ => { debug_assert!(!self.opcode().is_branch()); - None + &[] } } } - /// Get a mutable reference to the single destination of this branch instruction, if it is a - /// single destination branch or jump. + /// Get a mutable slice of the destinations of this instruction, if it's a branch. /// - /// Multi-destination branches like `br_table` return `None`. - pub fn branch_destination_mut(&mut self) -> Option<&mut BlockCall> { - match *self { + /// `br_table` returns the empty slice. + pub fn branch_destination_mut(&mut self) -> &mut [BlockCall] { + match self { Self::Jump { ref mut destination, .. @@ -309,11 +317,12 @@ impl InstructionData { | Self::Branch { ref mut destination, .. - } => Some(destination), - Self::BranchTable { .. } => None, + } => std::slice::from_mut(destination), + Self::Brif { blocks, .. } => blocks, + Self::BranchTable { .. } => &mut [], _ => { debug_assert!(!self.opcode().is_branch()); - None + &mut [] } } } @@ -456,6 +465,9 @@ pub enum BranchInfo { /// This is a branch or jump to a single destination block, possibly taking value arguments. SingleDest(BlockCall), + /// This is a conditional branch + Conditional(BlockCall, BlockCall), + /// This is a jump table branch which can have many destination blocks and one default block. Table(JumpTable, Block), } diff --git a/cranelift/codegen/src/isa/aarch64/lower.isle b/cranelift/codegen/src/isa/aarch64/lower.isle index 2f2a2ecfa3..14b5c04044 100644 --- a/cranelift/codegen/src/isa/aarch64/lower.isle +++ b/cranelift/codegen/src/isa/aarch64/lower.isle @@ -2461,6 +2461,52 @@ (rule (lower (fvpromote_low val)) (vec_rr_long (VecRRLongOp.Fcvtl32) val $false)) +;;; Rules for `brif` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +;; `brif` following `icmp` +(rule (lower_branch (brif (maybe_uextend (icmp cc x @ (value_type ty) y)) _ _) targets) + (let ((comparison FlagsAndCC (lower_icmp_into_flags cc x y ty)) + (cond Cond (cond_code (flags_and_cc_cc comparison))) + (taken BranchTarget (branch_target targets 0)) + (not_taken BranchTarget (branch_target targets 1))) + (emit_side_effect + (with_flags_side_effect (flags_and_cc_flags comparison) + (cond_br taken + not_taken + (cond_br_cond cond)))))) + +;; `brif` following `fcmp` +(rule (lower_branch (brif (maybe_uextend (fcmp cc x @ (value_type (ty_scalar_float ty)) y)) _ _) targets) + (let ((cond Cond (fp_cond_code cc)) + (taken BranchTarget (branch_target targets 0)) + (not_taken BranchTarget (branch_target targets 1))) + (emit_side_effect + (with_flags_side_effect (fpu_cmp (scalar_size ty) x y) + (cond_br taken not_taken + (cond_br_cond cond)))))) + +;; standard `brif` +(rule -1 (lower_branch (brif c @ (value_type $I128) _ _) targets) + (let ((flags ProducesFlags (flags_to_producesflags c)) + (c ValueRegs (put_in_regs c)) + (c_lo Reg (value_regs_get c 0)) + (c_hi Reg (value_regs_get c 1)) + (rt Reg (orr $I64 c_lo c_hi)) + (taken BranchTarget (branch_target targets 0)) + (not_taken BranchTarget (branch_target targets 1))) + (emit_side_effect + (with_flags_side_effect flags + (cond_br taken not_taken (cond_br_not_zero rt)))))) +(rule -2 (lower_branch (brif c @ (value_type ty) _ _) targets) + (if (ty_int_ref_scalar_64 ty)) + (let ((flags ProducesFlags (flags_to_producesflags c)) + (rt Reg (put_in_reg_zext64 c)) + (taken BranchTarget (branch_target targets 0)) + (not_taken BranchTarget (branch_target targets 1))) + (emit_side_effect + (with_flags_side_effect flags + (cond_br taken not_taken (cond_br_not_zero rt)))))) + ;;; Rules for `brz`/`brnz` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; `brz` following `icmp` diff --git a/cranelift/codegen/src/isa/aarch64/lower/isle.rs b/cranelift/codegen/src/isa/aarch64/lower/isle.rs index 263e6cc5b5..69796c4bc6 100644 --- a/cranelift/codegen/src/isa/aarch64/lower/isle.rs +++ b/cranelift/codegen/src/isa/aarch64/lower/isle.rs @@ -24,8 +24,8 @@ use crate::machinst::{isle::*, InputSourceInst}; use crate::{ binemit::CodeOffset, ir::{ - immediates::*, types::*, AtomicRmwOp, ExternalName, Inst, InstructionData, MemFlags, - TrapCode, Value, ValueList, + immediates::*, types::*, AtomicRmwOp, BlockCall, ExternalName, Inst, InstructionData, + MemFlags, TrapCode, Value, ValueList, }, isa::aarch64::abi::AArch64Caller, isa::aarch64::inst::args::{ShiftOp, ShiftOpShiftImm}, diff --git a/cranelift/codegen/src/isa/riscv64/inst.isle b/cranelift/codegen/src/isa/riscv64/inst.isle index 2031cf924a..6dc185014d 100644 --- a/cranelift/codegen/src/isa/riscv64/inst.isle +++ b/cranelift/codegen/src/isa/riscv64/inst.isle @@ -1898,6 +1898,30 @@ (hi Reg (value_regs_get regs 1))) (alu_rrr (AluOPRRR.Or) lo hi))) +;; Default behavior for branching based on an input value. +(rule + (lower_branch (brif v @ (value_type ty) _ _) targets) + (lower_brz_or_nz (IntCC.NotEqual) (normalize_cmp_value ty v) targets ty)) + +;; Special case for SI128 to reify the comparison value and branch on it. +(rule 2 + (lower_branch (brif v @ (value_type $I128) _ _) targets) + (let ((zero ValueRegs (value_regs (zero_reg) (zero_reg))) + (cmp Reg (gen_icmp (IntCC.NotEqual) v zero $I128))) + (lower_brz_or_nz (IntCC.NotEqual) cmp targets $I64))) + +;; Branching on the result of an icmp +(rule 1 + (lower_branch (brif (icmp cc a @ (value_type ty) b) _ _) targets) + (lower_br_icmp cc a b targets ty)) + +;; Branching on the result of an fcmp +(rule 1 + (lower_branch (brif (fcmp cc a @ (value_type ty) b) _ _) targets) + (let ((then BranchTarget (label_to_br_target (vec_label_get targets 0))) + (else BranchTarget (label_to_br_target (vec_label_get targets 1)))) + (emit_side_effect (cond_br (emit_fcmp cc ty a b) then else)))) + ;;;;; (rule (lower_branch (brz v @ (value_type ty) _) targets) diff --git a/cranelift/codegen/src/isa/riscv64/lower/isle.rs b/cranelift/codegen/src/isa/riscv64/lower/isle.rs index c41469eff6..125f005ae7 100644 --- a/cranelift/codegen/src/isa/riscv64/lower/isle.rs +++ b/cranelift/codegen/src/isa/riscv64/lower/isle.rs @@ -14,8 +14,8 @@ use crate::machinst::{isle::*, MachInst, SmallInstVec}; use crate::machinst::{VCodeConstant, VCodeConstantData}; use crate::{ ir::{ - immediates::*, types::*, AtomicRmwOp, ExternalName, Inst, InstructionData, MemFlags, - StackSlot, TrapCode, Value, ValueList, + immediates::*, types::*, AtomicRmwOp, BlockCall, ExternalName, Inst, InstructionData, + MemFlags, StackSlot, TrapCode, Value, ValueList, }, isa::riscv64::inst::*, machinst::{ArgPair, InstOutput, Lower}, diff --git a/cranelift/codegen/src/isa/s390x/lower.isle b/cranelift/codegen/src/isa/s390x/lower.isle index 96bffdf350..1819d060f8 100644 --- a/cranelift/codegen/src/isa/s390x/lower.isle +++ b/cranelift/codegen/src/isa/s390x/lower.isle @@ -3750,6 +3750,17 @@ (emit_side_effect (jt_sequence (lshl_imm $I64 idx 2) targets)))) +;;;; Rules for `brif` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +;; Two-way conditional branch on nonzero. `targets` contains: +;; - element 0: target if the condition is true (i.e. value is nonzero) +;; - element 1: target if the condition is false (i.e. value is zero) +(rule (lower_branch (brif val_cond _ _) targets) + (emit_side_effect (cond_br_bool (value_nonzero val_cond) + (vec_element targets 0) + (vec_element targets 1)))) + + ;;;; Rules for `brz` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; Two-way conditional branch on zero. `targets` contains: diff --git a/cranelift/codegen/src/isa/s390x/lower/isle.rs b/cranelift/codegen/src/isa/s390x/lower/isle.rs index df615b36c7..520cd5f371 100644 --- a/cranelift/codegen/src/isa/s390x/lower/isle.rs +++ b/cranelift/codegen/src/isa/s390x/lower/isle.rs @@ -16,8 +16,8 @@ use crate::machinst::isle::*; use crate::machinst::{MachLabel, Reg}; use crate::{ ir::{ - condcodes::*, immediates::*, types::*, ArgumentPurpose, AtomicRmwOp, Endianness, Inst, - InstructionData, KnownSymbol, LibCall, MemFlags, Opcode, TrapCode, Value, ValueList, + condcodes::*, immediates::*, types::*, ArgumentPurpose, AtomicRmwOp, BlockCall, Endianness, + Inst, InstructionData, KnownSymbol, LibCall, MemFlags, Opcode, TrapCode, Value, ValueList, }, isa::unwind::UnwindInst, isa::CallConv, diff --git a/cranelift/codegen/src/isa/x64/lower.isle b/cranelift/codegen/src/isa/x64/lower.isle index 191e703f8c..ec77a7fc78 100644 --- a/cranelift/codegen/src/isa/x64/lower.isle +++ b/cranelift/codegen/src/isa/x64/lower.isle @@ -2885,6 +2885,25 @@ (rule (lower_branch (jump _) (single_target target)) (emit_side_effect (jmp_known target))) +;; Rules for `brif` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(rule 2 (lower_branch (brif (maybe_uextend (icmp cc a b)) _ _) (two_targets then else)) + (emit_side_effect (jmp_cond_icmp (emit_cmp cc a b) then else))) + +(rule 2 (lower_branch (brif (maybe_uextend (fcmp cc a b)) _ _) (two_targets then else)) + (emit_side_effect (jmp_cond_fcmp (emit_fcmp cc a b) then else))) + +(rule 1 (lower_branch (brif val @ (value_type $I128) _ _) + (two_targets then else)) + (emit_side_effect (jmp_cond_icmp (cmp_zero_i128 (CC.Z) val) then else))) + +(rule (lower_branch (brif val @ (value_type (ty_int_bool_or_ref)) _ _) + (two_targets then else)) + (emit_side_effect (with_flags_side_effect + (cmp_zero_int_bool_ref val) + (jmp_cond (CC.NZ) then else)))) + + ;; Rules for `brz` and `brnz` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; (rule 2 (lower_branch (brz (maybe_uextend (icmp cc a b)) _) (two_targets taken not_taken)) diff --git a/cranelift/codegen/src/isa/x64/lower/isle.rs b/cranelift/codegen/src/isa/x64/lower/isle.rs index b32eb3cb30..7d97d761ed 100644 --- a/cranelift/codegen/src/isa/x64/lower/isle.rs +++ b/cranelift/codegen/src/isa/x64/lower/isle.rs @@ -20,7 +20,7 @@ use crate::{ condcodes::{CondCode, FloatCC, IntCC}, immediates::*, types::*, - Inst, InstructionData, MemFlags, Opcode, TrapCode, Value, ValueList, + BlockCall, Inst, InstructionData, MemFlags, Opcode, TrapCode, Value, ValueList, }, isa::{ unwind::UnwindInst, diff --git a/cranelift/codegen/src/isle_prelude.rs b/cranelift/codegen/src/isle_prelude.rs index 58f42f4868..8ee2d707df 100644 --- a/cranelift/codegen/src/isle_prelude.rs +++ b/cranelift/codegen/src/isle_prelude.rs @@ -637,5 +637,16 @@ macro_rules! isle_common_prelude_methods { fn pack_value_array_3(&mut self, a: Value, b: Value, c: Value) -> ValueArray3 { [a, b, c] } + + #[inline] + fn unpack_block_array_2(&mut self, arr: &BlockArray2) -> (BlockCall, BlockCall) { + let [a, b] = *arr; + (a, b) + } + + #[inline] + fn pack_block_array_2(&mut self, a: BlockCall, b: BlockCall) -> BlockArray2 { + [a, b] + } }; } diff --git a/cranelift/codegen/src/machinst/isle.rs b/cranelift/codegen/src/machinst/isle.rs index 30fcd7c24b..70bcb7d12e 100644 --- a/cranelift/codegen/src/machinst/isle.rs +++ b/cranelift/codegen/src/machinst/isle.rs @@ -1,4 +1,4 @@ -use crate::ir::{Value, ValueList}; +use crate::ir::{BlockCall, Value, ValueList}; use alloc::boxed::Box; use alloc::vec::Vec; use smallvec::SmallVec; @@ -22,6 +22,7 @@ pub type Unit = (); pub type ValueSlice = (ValueList, usize); pub type ValueArray2 = [Value; 2]; pub type ValueArray3 = [Value; 3]; +pub type BlockArray2 = [BlockCall; 2]; pub type WritableReg = Writable; pub type VecRetPair = Vec; pub type VecMask = Vec; diff --git a/cranelift/codegen/src/machinst/lower.rs b/cranelift/codegen/src/machinst/lower.rs index 3bdce4abd8..ea29cb3468 100644 --- a/cranelift/codegen/src/machinst/lower.rs +++ b/cranelift/codegen/src/machinst/lower.rs @@ -9,9 +9,9 @@ use crate::entity::SecondaryMap; use crate::fx::{FxHashMap, FxHashSet}; use crate::inst_predicates::{has_lowering_side_effect, is_constant_64bit}; use crate::ir::{ - ArgumentPurpose, Block, Constant, ConstantData, DataFlowGraph, ExternalName, Function, - GlobalValue, GlobalValueData, Immediate, Inst, InstructionData, MemFlags, Opcode, RelSourceLoc, - Type, Value, ValueDef, ValueLabelAssignments, ValueLabelStart, + instructions, ArgumentPurpose, Block, Constant, ConstantData, DataFlowGraph, ExternalName, + Function, GlobalValue, GlobalValueData, Immediate, Inst, InstructionData, MemFlags, Opcode, + RelSourceLoc, Type, Value, ValueDef, ValueLabelAssignments, ValueLabelStart, }; use crate::machinst::{ writable_value_regs, BlockIndex, BlockLoweringOrder, Callee, LoweredBlock, MachLabel, Reg, @@ -941,12 +941,28 @@ impl<'func, I: VCodeInst> Lower<'func, I> { for succ_idx in 0..self.vcode.block_order().succ_indices(block).len() { // Avoid immutable borrow by explicitly indexing. let (inst, succ) = self.vcode.block_order().succ_indices(block)[succ_idx]; - // Get branch args and convert to Regs. - let branch_args = self.f.dfg.insts[inst] - .branch_destination() - .into_iter() - .flat_map(|block| block.args_slice(&self.f.dfg.value_lists)); + // Get branch args and convert to Regs. + let branch_args = match self.f.dfg.analyze_branch(inst) { + instructions::BranchInfo::NotABranch => unreachable!(), + instructions::BranchInfo::SingleDest(block) => { + block.args_slice(&self.f.dfg.value_lists) + } + instructions::BranchInfo::Conditional(then_block, else_block) => { + // NOTE: `succ_idx == 0` implying that we're traversing the `then_block` is + // enforced by the traversal order defined in `visit_block_succs`. Eventually + // we should traverse the `branch_destination` slice instead of the result of + // analyze_branch there, which would simplify computing the branch args + // significantly. + if succ_idx == 0 { + then_block.args_slice(&self.f.dfg.value_lists) + } else { + assert!(succ_idx == 1); + else_block.args_slice(&self.f.dfg.value_lists) + } + } + instructions::BranchInfo::Table(_, _) => &[], + }; let mut branch_arg_vregs: SmallVec<[Reg; 16]> = smallvec![]; for &arg in branch_args { let arg = self.f.dfg.resolve_aliases(arg); @@ -976,7 +992,10 @@ impl<'func, I: VCodeInst> Lower<'func, I> { if last_inst != Some(inst) { branches.push(inst); } else { - debug_assert!(self.f.dfg.insts[inst].opcode() == Opcode::BrTable); + debug_assert!( + self.f.dfg.insts[inst].opcode() == Opcode::BrTable + || self.f.dfg.insts[inst].opcode() == Opcode::Brif + ); debug_assert!(branches.len() == 1); } last_inst = Some(inst); diff --git a/cranelift/codegen/src/prelude.isle b/cranelift/codegen/src/prelude.isle index 5a0aa3376c..77f7ccddf6 100644 --- a/cranelift/codegen/src/prelude.isle +++ b/cranelift/codegen/src/prelude.isle @@ -34,6 +34,7 @@ (type Type (primitive Type)) (type Value (primitive Value)) (type ValueList (primitive ValueList)) +(type BlockCall (primitive BlockCall)) ;; ISLE representation of `&[Value]`. (type ValueSlice (primitive ValueSlice)) diff --git a/cranelift/codegen/src/remove_constant_phis.rs b/cranelift/codegen/src/remove_constant_phis.rs index 4f03c7d8ca..4a9ef40170 100644 --- a/cranelift/codegen/src/remove_constant_phis.rs +++ b/cranelift/codegen/src/remove_constant_phis.rs @@ -4,7 +4,6 @@ use crate::dominator_tree::DominatorTree; use crate::fx::FxHashMap; use crate::fx::FxHashSet; use crate::ir; -use crate::ir::instructions::BranchInfo; use crate::ir::Function; use crate::ir::{Block, BlockCall, Inst, Value}; use crate::timing; @@ -112,6 +111,9 @@ impl AbstractValue { struct OutEdge<'a> { /// An instruction that transfers control. inst: Inst, + /// The index into branch_destinations for this instruction that corresponds + /// to this edge. + branch_index: u32, /// The block that control is transferred to. block: Block, /// The arguments to that block. @@ -126,7 +128,13 @@ impl<'a> OutEdge<'a> { /// Returns `None` if this is an edge without any block arguments, which /// means we can ignore it for this analysis's purposes. #[inline] - fn new(bump: &'a Bump, dfg: &ir::DataFlowGraph, inst: Inst, block: BlockCall) -> Option { + fn new( + bump: &'a Bump, + dfg: &ir::DataFlowGraph, + inst: Inst, + branch_index: usize, + block: BlockCall, + ) -> Option { let inst_var_args = block.args_slice(&dfg.value_lists); // Skip edges without params. @@ -136,6 +144,7 @@ impl<'a> OutEdge<'a> { Some(OutEdge { inst, + branch_index: branch_index as u32, block: block.block(&dfg.value_lists), args: bump.alloc_slice_fill_iter( inst_var_args @@ -231,12 +240,8 @@ pub fn do_remove_constant_phis(func: &mut Function, domtree: &mut DominatorTree) let mut summary = BlockSummary::new(&bump, formals); for inst in func.layout.block_insts(b) { - let idetails = &func.dfg.insts[inst]; - // Note that multi-dest transfers (i.e., branch tables) don't - // carry parameters in our IR, so we only have to care about - // `SingleDest` here. - if let BranchInfo::SingleDest(dest) = idetails.analyze_branch() { - if let Some(edge) = OutEdge::new(&bump, &func.dfg, inst, dest) { + for (ix, dest) in func.dfg.insts[inst].branch_destination().iter().enumerate() { + if let Some(edge) = OutEdge::new(&bump, &func.dfg, inst, ix, *dest) { summary.dests.push(edge); } } @@ -378,7 +383,9 @@ pub fn do_remove_constant_phis(func: &mut Function, domtree: &mut DominatorTree) } let dfg = &mut func.dfg; - let block = dfg.insts[edge.inst].branch_destination_mut().unwrap(); + let block = + &mut dfg.insts[edge.inst].branch_destination_mut()[edge.branch_index as usize]; + old_actuals.extend(block.args_slice(&dfg.value_lists)); // Check that the numbers of arguments make sense. diff --git a/cranelift/codegen/src/verifier/mod.rs b/cranelift/codegen/src/verifier/mod.rs index 396f3ff865..444b45c168 100644 --- a/cranelift/codegen/src/verifier/mod.rs +++ b/cranelift/codegen/src/verifier/mod.rs @@ -587,6 +587,15 @@ impl<'a> Verifier<'a> { Jump { destination, .. } | Branch { destination, .. } => { self.verify_block(inst, destination.block(&self.func.dfg.value_lists), errors)?; } + Brif { + arg, + blocks: [block_then, block_else], + .. + } => { + self.verify_value(inst, arg, errors)?; + self.verify_block(inst, block_then.block(&self.func.dfg.value_lists), errors)?; + self.verify_block(inst, block_else.block(&self.func.dfg.value_lists), errors)?; + } BranchTable { table, destination, .. } => { @@ -1303,6 +1312,25 @@ impl<'a> Verifier<'a> { let args = block.args_slice(&self.func.dfg.value_lists); self.typecheck_variable_args_iterator(inst, iter, args, errors)?; } + BranchInfo::Conditional(block_then, block_else) => { + let iter = self + .func + .dfg + .block_params(block_then.block(&self.func.dfg.value_lists)) + .iter() + .map(|&v| self.func.dfg.value_type(v)); + let args_then = block_then.args_slice(&self.func.dfg.value_lists); + self.typecheck_variable_args_iterator(inst, iter, args_then, errors)?; + + let iter = self + .func + .dfg + .block_params(block_else.block(&self.func.dfg.value_lists)) + .iter() + .map(|&v| self.func.dfg.value_type(v)); + let args_else = block_else.args_slice(&self.func.dfg.value_lists); + self.typecheck_variable_args_iterator(inst, iter, args_else, errors)?; + } BranchInfo::Table(table, block) => { let arg_count = self.func.dfg.num_block_params(block); if arg_count != 0 { diff --git a/cranelift/codegen/src/write.rs b/cranelift/codegen/src/write.rs index 49e6e76d29..34600b25ef 100644 --- a/cranelift/codegen/src/write.rs +++ b/cranelift/codegen/src/write.rs @@ -418,6 +418,16 @@ pub fn write_operands(w: &mut dyn Write, dfg: &DataFlowGraph, inst: Inst) -> fmt write!(w, " {}", destination.block(pool))?; write_block_args(w, destination.args_slice(pool)) } + Brif { + arg, + blocks: [block_then, block_else], + .. + } => { + write!(w, " {}, {}", arg, block_then.block(pool))?; + write_block_args(w, block_then.args_slice(pool))?; + write!(w, ", {}", block_else.block(pool))?; + write_block_args(w, block_else.args_slice(pool)) + } Branch { arg, destination, .. } => { diff --git a/cranelift/filetests/filetests/isa/x64/branches.clif b/cranelift/filetests/filetests/isa/x64/branches.clif index 1276b74369..8ed504d6c7 100644 --- a/cranelift/filetests/filetests/isa/x64/branches.clif +++ b/cranelift/filetests/filetests/isa/x64/branches.clif @@ -394,3 +394,65 @@ block2: ; popq %rbp ; ret +function %brif_i8_icmp(i32, i32) -> i32 { +block0(v0: i32, v1: i32): + v2 = icmp eq v0, v1 + v3 = uextend.i32 v2 + brif v3, block1, block2 + +block1: + v4 = iconst.i32 1 + return v4 + +block2: + v5 = iconst.i32 2 + return v5 +} + +; pushq %rbp +; movq %rsp, %rbp +; block0: +; cmpl %esi, %edi +; jz label1; j label2 +; block1: +; movl $1, %eax +; movq %rbp, %rsp +; popq %rbp +; ret +; block2: +; movl $2, %eax +; movq %rbp, %rsp +; popq %rbp +; ret + +function %brif_i8_fcmp(f32, f32) -> i32 { +block0(v0: f32, v1: f32): + v2 = fcmp eq v0, v1 + v3 = uextend.i32 v2 + brif v3, block1, block2 + +block1: + v4 = iconst.i32 1 + return v4 + +block2: + v5 = iconst.i32 2 + return v5 +} + +; pushq %rbp +; movq %rsp, %rbp +; block0: +; ucomiss %xmm1, %xmm0 +; jp label2 +; jnz label2; j label1 +; block1: +; movl $1, %eax +; movq %rbp, %rsp +; popq %rbp +; ret +; block2: +; movl $2, %eax +; movq %rbp, %rsp +; popq %rbp +; ret diff --git a/cranelift/filetests/filetests/parser/branch.clif b/cranelift/filetests/filetests/parser/branch.clif index c9a71312d9..4c7cd70c2a 100644 --- a/cranelift/filetests/filetests/parser/branch.clif +++ b/cranelift/filetests/filetests/parser/branch.clif @@ -114,3 +114,24 @@ block50: ; nextln: block50: ; nextln: trap user1 ; nextln: } + +function %twoargs(i32, f32) { +block0(v90: i32, v91: f32): + brif v90, block1(v90, v91), block2(v91, v90) + +block1(v92: i32, v93: f32): + brif v90, block1(v90, v91), block2(v91, v90) + +block2(v94: f32, v95: i32): + brif v90, block1(v90, v91), block2(v91, v90) +} +; sameln: function %twoargs(i32, f32) fast { +; nextln: block0(v90: i32, v91: f32): +; nextln: brif v90, block1(v90, v91), block2(v91, v90) +; nextln: +; nextln: block1(v92: i32, v93: f32): +; nextln: brif.i32 v90, block1(v90, v91), block2(v91, v90) +; nextln: +; nextln: block2(v94: f32, v95: i32): +; nextln: brif.i32 v90, block1(v90, v91), block2(v91, v90) +; nextln: } diff --git a/cranelift/filetests/filetests/runtests/brif.clif b/cranelift/filetests/filetests/runtests/brif.clif new file mode 100644 index 0000000000..b751ca09e6 --- /dev/null +++ b/cranelift/filetests/filetests/runtests/brif.clif @@ -0,0 +1,195 @@ +test interpret +test run +target aarch64 +target s390x +target x86_64 +target riscv64 + +function %brif_value(i8) -> i64 { +block0(v0: i8): + brif v0, block1, block2 +block1: + v1 = uextend.i64 v0 + return v1 +block2: + v2 = iconst.i64 42 + return v2 +} + +; run: %brif_value(0) == 42 +; run: %brif_value(42) == 42 +; run: %brif_value(97) == 97 + +function %brif_ne_zero(i8) -> i64 { +block0(v0: i8): + v1 = iconst.i8 0 + v2 = icmp ne v0, v1 + brif v2, block1, block2 +block1: + v3 = uextend.i64 v0 + return v3 +block2: + v4 = iconst.i64 42 + return v4 +} + +; run: %brif_ne_zero(0) == 42 +; run: %brif_ne_zero(42) == 42 +; run: %brif_ne_zero(97) == 97 + +function %brif_ne_one(i8) -> i64 { +block0(v0: i8): + v1 = iconst.i8 1 + v2 = icmp ne v0, v1 + brif v2, block1, block2 +block1: + v3 = uextend.i64 v0 + return v3 +block2: + v4 = iconst.i64 42 + return v4 +} + +; run: %brif_ne_one(1) == 42 +; run: %brif_ne_one(0) == 0 +; run: %brif_ne_one(42) == 42 +; run: %brif_ne_one(97) == 97 + +function %brif_uextend_ne_one(i8) -> i64 { +block0(v0: i8): + v1 = iconst.i8 1 + v2 = icmp ne v0, v1 + v3 = uextend.i64 v2 + brif v3, block1, block2 +block1: + v4 = uextend.i64 v0 + return v4 +block2: + v5 = iconst.i64 42 + return v5 +} + +; run: %brif_uextend_ne_one(1) == 42 +; run: %brif_uextend_ne_one(0) == 0 +; run: %brif_uextend_ne_one(42) == 42 +; run: %brif_uextend_ne_one(97) == 97 + + +function %brif_i64(i64) -> i8 { +block0(v0: i64): + brif v0, block1, block2 + +block1: + v1 = iconst.i8 1 + return v1 + +block2: + v2 = iconst.i8 0 + return v2 +} +; run: %brif_i64(0) == 0 +; run: %brif_i64(1) == 1 +; run: %brif_i64(-1) == 1 + +function %brif_i32(i32) -> i8 { +block0(v0: i32): + brif v0, block1, block2 + +block1: + v1 = iconst.i8 1 + return v1 + +block2: + v2 = iconst.i8 0 + return v2 +} +; run: %brif_i32(0) == 0 +; run: %brif_i32(1) == 1 +; run: %brif_i32(-1) == 1 + +function %brif_i16(i16) -> i8 { +block0(v0: i16): + brif v0, block1, block2 + +block1: + v1 = iconst.i8 1 + return v1 + +block2: + v2 = iconst.i8 0 + return v2 +} +; run: %brif_i16(0) == 0 +; run: %brif_i16(1) == 1 +; run: %brif_i16(-1) == 1 + +function %brif_i8(i8) -> i8 { +block0(v0: i8): + brif v0, block1, block2 + +block1: + v1 = iconst.i8 1 + return v1 + +block2: + v2 = iconst.i8 0 + return v2 +} +; run: %brif_i8(0) == 0 +; run: %brif_i8(1) == 1 +; run: %brif_i8(-1) == 1 +; run: %brif_i8(97) == 1 + +function %brif_different_args(i8) -> i8 { +block0(v0: i8): + brif v0, block1(v0, v0), block2(v0) + +block1(v1: i8, v2: i8): + v3 = iadd v1, v2 + return v3 + +block2(v4: i8): + return v4 +} + +; run: %brif_different_args(0) == 0 +; run: %brif_different_args(1) == 2 +; run: %brif_different_args(8) == 16 +; run: %brif_different_args(128) == 0 + +function %fuzzgen_1() -> i8 system_v { +block0: + v1 = iconst.i8 35 + brif v1, block1(v1), block1(v1) ; v1 = 35 + +block1(v0: i8): + return v0 +} + +; run: %fuzzgen_1() == 35 + +function %fuzzgen_2(i16) -> i16, i16 system_v { +block0(v0: i16): + brnz v0, block1(v0, v0) + jump block2(v0, v0) + +block1(v1: i16, v2: i16): + brif v1, block2(v2, v2), block2(v2, v2) + +block2(v3: i16, v4: i16): + return v3, v4 +} + +; run: %fuzzgen_2(0) == [0, 0] + +function %fuzzgen_3(i8 sext) -> i8 system_v { +block0(v0: i8): + v1 = iconst.i8 -9 + brif v0, block1(v1), block1(v0) + +block1(v2: i8): + return v2 +} + +; run: %fuzzgen_3(-65) == -9 +; run: %fuzzgen_3(0) == 0 diff --git a/cranelift/filetests/filetests/verifier/type_check.clif b/cranelift/filetests/filetests/verifier/type_check.clif index 0663f8fd7e..02f0589df7 100644 --- a/cranelift/filetests/filetests/verifier/type_check.clif +++ b/cranelift/filetests/filetests/verifier/type_check.clif @@ -98,6 +98,22 @@ function %jump_args2() { return } +function %brif_args() { +block0: + v0 = iconst.i16 10 + v1 = iconst.i16 10 + brif v0, block1(v1), block2(v1) + ; error: arg 0 (v1) has type i16, expected i64 + ; error: mismatched argument count + ; error: arg 0 (v1) has type i16, expected f32 + +block1(v2: i64): + return + +block2(v3: f32, v4: i8): + return +} + function %bad_extend() { block0: v0 = iconst.i32 10 diff --git a/cranelift/frontend/src/frontend.rs b/cranelift/frontend/src/frontend.rs index f25e6c37b1..dbef386f89 100644 --- a/cranelift/frontend/src/frontend.rs +++ b/cranelift/frontend/src/frontend.rs @@ -106,46 +106,48 @@ impl<'short, 'long> InstBuilderBase<'short> for FuncInstBuilder<'short, 'long> { self.builder.func.set_srcloc(inst, self.builder.srcloc); } - if data.opcode().is_branch() { - match data.branch_destination() { - Some(dest_block) => { - // If the user has supplied jump arguments we must adapt the arguments of - // the destination block - self.builder.declare_successor( - dest_block.block(&self.builder.func.dfg.value_lists), - inst, - ); + match self.builder.func.dfg.analyze_branch(inst) { + ir::instructions::BranchInfo::NotABranch => (), + + ir::instructions::BranchInfo::SingleDest(dest) => { + // If the user has supplied jump arguments we must adapt the arguments of + // the destination block + let block = dest.block(&self.builder.func.dfg.value_lists); + self.builder.declare_successor(block, inst); + } + + ir::instructions::BranchInfo::Conditional(branch_then, branch_else) => { + let block_then = branch_then.block(&self.builder.func.dfg.value_lists); + let block_else = branch_else.block(&self.builder.func.dfg.value_lists); + + self.builder.declare_successor(block_then, inst); + if block_then != block_else { + self.builder.declare_successor(block_else, inst); } - None => { - // branch_destination() doesn't detect jump_tables - // If jump table we declare all entries successor - if let InstructionData::BranchTable { - table, destination, .. - } = data - { - // Unlike all other jumps/branches, jump tables are - // capable of having the same successor appear - // multiple times, so we must deduplicate. - let mut unique = EntitySet::::new(); - for dest_block in self - .builder - .func - .jump_tables - .get(table) - .expect("you are referencing an undeclared jump table") - .iter() - .filter(|&dest_block| unique.insert(*dest_block)) - { - // Call `declare_block_predecessor` instead of `declare_successor` for - // avoiding the borrow checker. - self.builder - .func_ctx - .ssa - .declare_block_predecessor(*dest_block, inst); - } - self.builder.declare_successor(destination, inst); - } + } + + ir::instructions::BranchInfo::Table(table, destination) => { + // Unlike all other jumps/branches, jump tables are + // capable of having the same successor appear + // multiple times, so we must deduplicate. + let mut unique = EntitySet::::new(); + for dest_block in self + .builder + .func + .jump_tables + .get(table) + .expect("you are referencing an undeclared jump table") + .iter() + .filter(|&dest_block| unique.insert(*dest_block)) + { + // Call `declare_block_predecessor` instead of `declare_successor` for + // avoiding the borrow checker. + self.builder + .func_ctx + .ssa + .declare_block_predecessor(*dest_block, inst); } + self.builder.declare_successor(destination, inst); } } @@ -680,14 +682,13 @@ impl<'a> FunctionBuilder<'a> { /// other jump instructions. pub fn change_jump_destination(&mut self, inst: Inst, new_dest: Block) { let dfg = &mut self.func.dfg; - let old_dest = dfg.insts[inst] - .branch_destination_mut() - .expect("you want to change the jump destination of a non-jump instruction"); - self.func_ctx - .ssa - .remove_block_predecessor(old_dest.block(&dfg.value_lists), inst); - old_dest.set_block(new_dest, &mut dfg.value_lists); - self.func_ctx.ssa.declare_block_predecessor(new_dest, inst); + for old_dest in dfg.insts[inst].branch_destination_mut() { + self.func_ctx + .ssa + .remove_block_predecessor(old_dest.block(&dfg.value_lists), inst); + old_dest.set_block(new_dest, &mut dfg.value_lists); + self.func_ctx.ssa.declare_block_predecessor(new_dest, inst); + } } /// Returns `true` if and only if the current `Block` is sealed and has no predecessors declared. diff --git a/cranelift/frontend/src/ssa.rs b/cranelift/frontend/src/ssa.rs index 058c95d2b5..4e41f6346f 100644 --- a/cranelift/frontend/src/ssa.rs +++ b/cranelift/frontend/src/ssa.rs @@ -586,10 +586,18 @@ impl SSABuilder { // is sufficient. BranchInfo::SingleDest(_) => { let dfg = &mut func.dfg; - dfg.insts[branch] - .branch_destination_mut() - .unwrap() - .append_argument(val, &mut dfg.value_lists); + for dest in dfg.insts[branch].branch_destination_mut() { + dest.append_argument(val, &mut dfg.value_lists); + } + None + } + BranchInfo::Conditional(_, _) => { + let dfg = &mut func.dfg; + for block in dfg.insts[branch].branch_destination_mut() { + if block.block(&dfg.value_lists) == dest_block { + block.append_argument(val, &mut dfg.value_lists); + } + } None } BranchInfo::Table(mut jt, _default_block) => { diff --git a/cranelift/fuzzgen/src/function_generator.rs b/cranelift/fuzzgen/src/function_generator.rs index 1de8f15c61..23ba8f1de7 100644 --- a/cranelift/fuzzgen/src/function_generator.rs +++ b/cranelift/fuzzgen/src/function_generator.rs @@ -1615,12 +1615,25 @@ where let _type = *self.u.choose(&condbr_types[..])?; let val = builder.use_var(self.get_variable_of_type(_type)?); - if bool::arbitrary(self.u)? { - builder.ins().brz(val, left, &left_args[..]); - } else { - builder.ins().brnz(val, left, &left_args[..]); + match self.u.int_in_range(0..=2)? { + 0 => { + builder.ins().brnz(val, left, &left_args[..]); + builder.ins().jump(right, &right_args[..]); + } + + 1 => { + builder.ins().brz(val, left, &left_args[..]); + builder.ins().jump(right, &right_args[..]); + } + + 2 => { + builder + .ins() + .brif(val, left, &left_args[..], right, &right_args[..]); + } + + _ => unreachable!(), } - builder.ins().jump(right, &right_args[..]); } BlockTerminator::BrTable(default, targets) => { // Create jump tables on demand diff --git a/cranelift/interpreter/src/step.rs b/cranelift/interpreter/src/step.rs index 89a63bae7e..d791ee6f87 100644 --- a/cranelift/interpreter/src/step.rs +++ b/cranelift/interpreter/src/step.rs @@ -7,8 +7,8 @@ use crate::value::{Value, ValueConversionKind, ValueError, ValueResult}; use cranelift_codegen::data_value::DataValue; use cranelift_codegen::ir::condcodes::{FloatCC, IntCC}; use cranelift_codegen::ir::{ - types, AbiParam, Block, ExternalName, FuncRef, Function, InstructionData, Opcode, TrapCode, - Type, Value as ValueRef, + types, AbiParam, Block, BlockCall, ExternalName, FuncRef, Function, InstructionData, Opcode, + TrapCode, Type, Value as ValueRef, }; use log::trace; use smallvec::{smallvec, SmallVec}; @@ -236,8 +236,7 @@ where // Retrieve an instruction's branch destination; expects the instruction to be a branch. - let continue_at = || { - let block = inst.branch_destination().unwrap(); + let continue_at = |block: BlockCall| { let branch_args = state .collect_values(block.args_slice(&state.get_current_function().dfg.value_lists)) .map_err(|v| StepError::UnknownValue(v))?; @@ -248,9 +247,9 @@ where }; // Based on `condition`, indicate where to continue the control flow. - let branch_when = |condition: bool| -> Result, StepError> { + let branch_when = |condition: bool, block| -> Result, StepError> { if condition { - continue_at() + continue_at(block) } else { Ok(ControlFlow::Continue) } @@ -279,17 +278,48 @@ where // Interpret a Cranelift instruction. Ok(match inst.opcode() { - Opcode::Jump => continue_at()?, - Opcode::Brz => branch_when( - !arg(0)? - .convert(ValueConversionKind::ToBoolean)? - .into_bool()?, - )?, - Opcode::Brnz => branch_when( - arg(0)? - .convert(ValueConversionKind::ToBoolean)? - .into_bool()?, - )?, + Opcode::Jump => { + let block = inst.branch_destination()[0]; + continue_at(block)? + } + Opcode::Brif => { + if let InstructionData::Brif { + arg, + blocks: [block_then, block_else], + .. + } = inst + { + let arg = state.get_value(arg).ok_or(StepError::UnknownValue(arg))?; + + let condition = arg.convert(ValueConversionKind::ToBoolean)?.into_bool()?; + + if condition { + continue_at(block_then)? + } else { + continue_at(block_else)? + } + } else { + unreachable!() + } + } + Opcode::Brz => { + let block = inst.branch_destination()[0]; + branch_when( + !arg(0)? + .convert(ValueConversionKind::ToBoolean)? + .into_bool()?, + block, + )? + } + Opcode::Brnz => { + let block = inst.branch_destination()[0]; + branch_when( + arg(0)? + .convert(ValueConversionKind::ToBoolean)? + .into_bool()?, + block, + )? + } Opcode::BrTable => { if let InstructionData::BranchTable { table, destination, .. diff --git a/cranelift/reader/src/parser.rs b/cranelift/reader/src/parser.rs index 1a6b05d2c8..e55ba1bf85 100644 --- a/cranelift/reader/src/parser.rs +++ b/cranelift/reader/src/parser.rs @@ -2588,6 +2588,26 @@ impl<'a> Parser<'a> { destination, } } + InstructionFormat::Brif => { + let arg = self.match_value("expected SSA value control operand")?; + self.match_token(Token::Comma, "expected ',' between operands")?; + let block_then = { + let block_num = self.match_block("expected branch then block")?; + let args = self.parse_opt_value_list()?; + ctx.function.dfg.block_call(block_num, &args) + }; + self.match_token(Token::Comma, "expected ',' between operands")?; + let block_else = { + let block_num = self.match_block("expected branch else block")?; + let args = self.parse_opt_value_list()?; + ctx.function.dfg.block_call(block_num, &args) + }; + InstructionData::Brif { + opcode, + arg, + blocks: [block_then, block_else], + } + } InstructionFormat::Branch => { let arg = self.match_value("expected SSA value control operand")?; self.match_token(Token::Comma, "expected ',' between operands")?;