From d1b8937fd1795dcb1cdcd40c6c1a140e6daf2ae0 Mon Sep 17 00:00:00 2001 From: Aaron Orenstein Date: Sun, 15 Jan 2023 09:24:52 -0800 Subject: [PATCH] Handle rewriting multi-return instrs Summary: When performing a rewrite if the instruction being rewritten is multi-return and is followed by Select instructions then the rewriter needs to know how to route the Select InstrIds. This change (a) detects if the Selects were not re-routed properly. (b) automatically reroutes them in simple cases. (c) allows more complex cases. Reviewed By: edwinsmith Differential Revision: D42441069 fbshipit-source-id: 03520b409b1112505f33e469dcbe3dcb16330596 --- .../hackc/ir/conversions/textual/lower/instrs.rs | 93 +++++++++++++++++---- hphp/hack/src/hackc/ir/ir_core/func_builder.rs | 96 +++++++++++++++++++++- hphp/hack/src/hackc/ir/passes/ssa.rs | 9 +- 3 files changed, 178 insertions(+), 20 deletions(-) diff --git a/hphp/hack/src/hackc/ir/conversions/textual/lower/instrs.rs b/hphp/hack/src/hackc/ir/conversions/textual/lower/instrs.rs index f3c093b1bbb..f5eab73a0c2 100644 --- a/hphp/hack/src/hackc/ir/conversions/textual/lower/instrs.rs +++ b/hphp/hack/src/hackc/ir/conversions/textual/lower/instrs.rs @@ -6,6 +6,7 @@ use std::sync::Arc; use ir::func_builder::TransformInstr; +use ir::func_builder::TransformState; use ir::instr::CallDetail; use ir::instr::CmpOp; use ir::instr::HasLoc; @@ -13,6 +14,7 @@ use ir::instr::HasOperands; use ir::instr::Hhbc; use ir::instr::IteratorArgs; use ir::instr::Predicate; +use ir::instr::Special; use ir::instr::Terminator; use ir::instr::Textual; use ir::BareThisOp; @@ -278,7 +280,13 @@ impl LowerInstrs<'_> { } impl TransformInstr for LowerInstrs<'_> { - fn apply(&mut self, _iid: InstrId, instr: Instr, builder: &mut FuncBuilder<'_>) -> Instr { + fn apply( + &mut self, + iid: InstrId, + instr: Instr, + builder: &mut FuncBuilder<'_>, + state: &mut TransformState, + ) -> Instr { use hack::Builtin; if let Some(instr) = self.handle_with_builtin(builder, &instr) { @@ -314,7 +322,7 @@ impl TransformInstr for LowerInstrs<'_> { }, .. }, - ) => rewrite_nullsafe_call(builder, call), + ) => rewrite_nullsafe_call(iid, builder, call, state), Instr::Hhbc(Hhbc::BareThis(_op, loc)) => { let this = builder.strings.intern_str("$this"); let lid = LocalId::Named(this); @@ -450,7 +458,12 @@ impl TransformInstr for LowerInstrs<'_> { } } -fn rewrite_nullsafe_call(builder: &mut FuncBuilder<'_>, mut call: Call) -> Instr { +fn rewrite_nullsafe_call( + original_iid: InstrId, + builder: &mut FuncBuilder<'_>, + mut call: Call, + state: &mut TransformState, +) -> Instr { // rewrite a call like `$a?->b()` into `$a ? $a->b() : null` call.detail = match call.detail { CallDetail::FCallObjMethod { .. } => CallDetail::FCallObjMethod { @@ -463,23 +476,73 @@ fn rewrite_nullsafe_call(builder: &mut FuncBuilder<'_>, mut call: Call) -> Instr _ => unreachable!(), }; + // We need to be careful about multi-return! + // (ret, a, b) = obj?->call(inout a, inout b) + // -> + // if (obj) { + // (ret, a, b) + // } else { + // (null, a, b) + // } + let loc = call.loc; let obj = call.obj(); + let num_rets = call.num_rets; let pred = builder.emit_hack_builtin(hack::Builtin::Hhbc(hack::Hhbc::IsTypeNull), &[obj], loc); - let res = builder.emit_if_then_else( + + // Need to customize the if/then/else because of multi-return. + let join_bid = builder.alloc_bid(); + let null_bid = builder.alloc_bid(); + let nonnull_bid = builder.alloc_bid(); + builder.emit(Instr::jmp_op( pred, + Predicate::NonZero, + null_bid, + nonnull_bid, loc, - |builder| { - // is null - builder.emit_constant(Constant::Null) - }, - |builder| { - // is not null - builder.emit(Instr::Call(Box::new(call))) - }, - ); - - Instr::copy(res) + )); + + // Null case - main value should be null. Inouts should be passed through. + builder.start_block(null_bid); + let mut args = Vec::with_capacity(num_rets as usize); + args.push(builder.emit_constant(Constant::Null)); + if let Some(inouts) = call.inouts.as_ref() { + for inout_idx in inouts.iter() { + let op = call.operands[*inout_idx as usize]; + args.push(op); + } + } + builder.emit(Instr::jmp_args(join_bid, &args, loc)); + + // Nonnull case - make call and pass Select values. + builder.start_block(nonnull_bid); + args.clear(); + let iid = builder.emit(Instr::Call(Box::new(call))); + if num_rets > 1 { + for idx in 0..num_rets { + args.push(builder.emit(Instr::Special(Special::Select(iid, idx)))); + } + } else { + args.push(iid); + } + builder.emit(Instr::jmp_args(join_bid, &args, loc)); + + // Join + builder.start_block(join_bid); + args.clear(); + if num_rets > 1 { + // we need to swap out the original Select statements. + for idx in 0..num_rets { + let param = builder.alloc_param(); + args.push(param); + state.rewrite_select_idx(original_iid, idx as usize, param); + } + Instr::tombstone() + } else { + let param = builder.alloc_param(); + args.push(param); + Instr::copy(args[0]) + } } fn iter_var_name(id: ir::IterId, strings: &ir::StringInterner) -> LocalId { diff --git a/hphp/hack/src/hackc/ir/ir_core/func_builder.rs b/hphp/hack/src/hackc/ir/ir_core/func_builder.rs index a8322095ab7..fe4853ed947 100644 --- a/hphp/hack/src/hackc/ir/ir_core/func_builder.rs +++ b/hphp/hack/src/hackc/ir/ir_core/func_builder.rs @@ -9,6 +9,7 @@ use hash::HashMap; use crate::func::SrcLoc; use crate::instr::HasOperands; +use crate::instr::Hhbc; use crate::instr::Special; use crate::Block; use crate::BlockId; @@ -191,8 +192,10 @@ impl<'a> FuncBuilder<'a> { self.cur_bid = bid; self.block_rewrite_stopped = false; + let mut state = TransformState::new(); + for iid in block_snapshot { - self.rewrite_instr(iid, transformer); + self.rewrite_instr(iid, transformer, &mut state); if self.block_rewrite_stopped { break; @@ -207,7 +210,12 @@ impl<'a> FuncBuilder<'a> { self.block_rewrite_stopped = true; } - fn rewrite_instr(&mut self, iid: InstrId, transformer: &mut dyn TransformInstr) { + fn rewrite_instr( + &mut self, + iid: InstrId, + transformer: &mut dyn TransformInstr, + state: &mut TransformState, + ) { // Temporarily steal from the func the instr we want to transform, so we // can manipulate it without borrowing the func. If we borrow it, we // wouldn't be able to create new instrs as part of the transformation. @@ -220,8 +228,16 @@ impl<'a> FuncBuilder<'a> { // Snap through any relaced instrs to the real value. self.replace_operands(&mut instr); + let output = match instr { + Instr::Special(Special::Select(src, idx)) => { + let src = src.expect_instr("instr expected"); + self.rewrite_select(iid, instr, src, idx, state) + } + _ => transformer.apply(iid, instr, self, state), + }; + // Apply the transformation and learn what to do with the result. - match transformer.apply(iid, instr, self) { + match output { Instr::Special(Special::Copy(value)) => { // Do copy-propagation on the fly, emitting nothing to the block. self.changed = true; @@ -235,6 +251,41 @@ impl<'a> FuncBuilder<'a> { } } + fn num_selects(instr: &Instr) -> u32 { + let num_rets = match instr { + Instr::Call(call) => call.num_rets, + Instr::Hhbc(Hhbc::ClassGetTS(..)) => 2, + Instr::MemberOp(ref op) => op.num_values() as u32, + _ => 1, + }; + if num_rets > 1 { num_rets } else { 0 } + } + + fn rewrite_select( + &mut self, + iid: InstrId, + instr: Instr, + src: InstrId, + idx: u32, + state: &mut TransformState, + ) -> Instr { + if let Some(dst) = state.select_mapping.get(&iid) { + // This acts like the Select was just replaced by Instr::copy(). + return Instr::copy(*dst); + } + + if iid == InstrId::from_usize(src.as_usize() + 1 + idx as usize) { + if idx < Self::num_selects(&self.func.instrs[src]) { + // If the rewriter didn't do anything interesting with the + // original Instr then we just want to keep the Select + // as-is. + return instr; + } + } + + panic!("Un-rewritten Select: InstrId {iid:?} ({instr:?})"); + } + /// If iid has been replaced with another InstrId, return that, else return vid. pub fn find_replacement(&mut self, vid: ValueId) -> ValueId { let replacements = &mut self.replacements; @@ -324,5 +375,42 @@ pub trait TransformInstr { /// /// - Any other instr is recorded in func.instrs[iid] and iid is appended to /// the current block being built up. - fn apply<'a>(&mut self, iid: InstrId, instr: Instr, builder: &mut FuncBuilder<'a>) -> Instr; + fn apply<'a>( + &mut self, + iid: InstrId, + instr: Instr, + builder: &mut FuncBuilder<'a>, + state: &mut TransformState, + ) -> Instr; +} + +/// Helper for TransformInstr +pub struct TransformState { + select_mapping: InstrIdMap, +} + +impl TransformState { + fn new() -> Self { + TransformState { + select_mapping: Default::default(), + } + } + + /// When rewriting an Instr that returns multiple values it is required to + /// call this function to rewrite the multiple return values. The inputs to + /// this function are the InstrId of the Instr::Special(Special::Select(..)). + pub fn rewrite_select(&mut self, src: InstrId, dst: ValueId) { + assert!(!self.select_mapping.contains_key(&src)); + self.select_mapping.insert(src, dst); + } + + /// Like rewrite_select() this is used to note multiple return values - but + /// uses the InstrId of the original instruction (not the Select) and a + /// Select index. + pub fn rewrite_select_idx(&mut self, src_iid: InstrId, src_idx: usize, dst_vid: ValueId) { + self.rewrite_select( + InstrId::from_usize(src_iid.as_usize() + 1 + src_idx), + dst_vid, + ); + } } diff --git a/hphp/hack/src/hackc/ir/passes/ssa.rs b/hphp/hack/src/hackc/ir/passes/ssa.rs index 204a00f3b0e..48a12d672a9 100644 --- a/hphp/hack/src/hackc/ir/passes/ssa.rs +++ b/hphp/hack/src/hackc/ir/passes/ssa.rs @@ -6,6 +6,7 @@ use analysis::PredecessorCatchMode; use analysis::PredecessorFlags; use analysis::Predecessors; use ir_core::func_builder::TransformInstr; +use ir_core::func_builder::TransformState; use ir_core::instr::Special; use ir_core::instr::Terminator; use ir_core::instr::Tmp; @@ -355,7 +356,13 @@ impl<'a> MakeSSA<'a> { } impl<'a> TransformInstr for MakeSSA<'a> { - fn apply(&mut self, _iid: InstrId, i: Instr, rw: &mut FuncBuilder<'_>) -> Instr { + fn apply( + &mut self, + _iid: InstrId, + i: Instr, + rw: &mut FuncBuilder<'_>, + _state: &mut TransformState, + ) -> Instr { let bid = rw.cur_bid(); match i { -- 2.11.4.GIT