From 9e9065262278b151de6bed5296e46138012a0b3a Mon Sep 17 00:00:00 2001 From: Ashley Hauck Date: Mon, 19 Oct 2020 16:11:45 +0200 Subject: [PATCH] Don't initialize vars to undef (#68) --- .../src/builder/builder_methods.rs | 12 +--- rustc_codegen_spirv/src/linker/inline.rs | 24 +------- rustc_codegen_spirv/src/linker/mem2reg.rs | 56 ++++++++++++++++++- rustc_codegen_spirv/src/linker/mod.rs | 1 + 4 files changed, 57 insertions(+), 36 deletions(-) diff --git a/rustc_codegen_spirv/src/builder/builder_methods.rs b/rustc_codegen_spirv/src/builder/builder_methods.rs index 0c6272b805..61e4a6392f 100644 --- a/rustc_codegen_spirv/src/builder/builder_methods.rs +++ b/rustc_codegen_spirv/src/builder/builder_methods.rs @@ -670,7 +670,6 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { pointee: ty, } .def(self); - let undef_ty = self.undef(ty); // "All OpVariable instructions in a function must be the first instructions in the first block." let mut builder = self.emit(); builder.select_block(Some(0)).unwrap(); @@ -696,16 +695,7 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { Op::Variable, Some(ptr_ty), Some(result_id), - vec![ - Operand::StorageClass(StorageClass::Function), - // TODO: Always include an undef initializer, because spir-v does not specify the - // value of an uninitialized variable. So, initialize it to undef. - // See #17 for tracking spec'ing this in spir-v: - // https://github.com/EmbarkStudios/rust-gpu/issues/17 - // (This also helps out linker/mem2reg.rs in some places - it gives it a handy - // source of a deduped OpUndef so it doesn't have to do that work itself) - Operand::IdRef(undef_ty.def), - ], + vec![Operand::StorageClass(StorageClass::Function)], ); builder.insert_into_block(index, inst).unwrap(); result_id.with_type(ptr_ty) diff --git a/rustc_codegen_spirv/src/linker/inline.rs b/rustc_codegen_spirv/src/linker/inline.rs index 4b4f342e7e..c74e23141c 100644 --- a/rustc_codegen_spirv/src/linker/inline.rs +++ b/rustc_codegen_spirv/src/linker/inline.rs @@ -170,21 +170,6 @@ impl Inliner<'_, '_> { inst_id } - fn undef_for(&mut self, ty: Word) -> Word { - // TODO: This is horribly slow, fix this - let existing = self - .types_global_values - .iter() - .find(|inst| inst.class.opcode == Op::Undef && inst.result_type.unwrap() == ty); - if let Some(existing) = existing { - return existing.result_id.unwrap(); - } - let inst_id = self.id(); - self.types_global_values - .push(Instruction::new(Op::Undef, Some(ty), Some(inst_id), vec![])); - inst_id - } - fn inline_fn(&mut self, function: &mut Function) { let mut block_idx = 0; while block_idx < function.blocks.len() { @@ -267,7 +252,6 @@ impl Inliner<'_, '_> { &mut caller.blocks[0], self.ptr_ty(call_result_type), return_variable.unwrap(), - self.undef_for(call_result_type), ); } @@ -360,7 +344,7 @@ fn get_inlined_blocks( blocks } -fn insert_opvariable(block: &mut Block, ptr_ty: Word, result_id: Word, undef_value: Word) { +fn insert_opvariable(block: &mut Block, ptr_ty: Word, result_id: Word) { let index = block .instructions .iter() @@ -376,11 +360,7 @@ fn insert_opvariable(block: &mut Block, ptr_ty: Word, result_id: Word, undef_val Op::Variable, Some(ptr_ty), Some(result_id), - vec![ - Operand::StorageClass(StorageClass::Function), - // See comment in `builder_methods.rs` `alloca` for why this undef is here. - Operand::IdRef(undef_value), - ], + vec![Operand::StorageClass(StorageClass::Function)], ); match index { Some(index) => block.instructions.insert(index, inst), diff --git a/rustc_codegen_spirv/src/linker/mem2reg.rs b/rustc_codegen_spirv/src/linker/mem2reg.rs index fe5f92647e..0fee842937 100644 --- a/rustc_codegen_spirv/src/linker/mem2reg.rs +++ b/rustc_codegen_spirv/src/linker/mem2reg.rs @@ -17,6 +17,7 @@ use std::collections::{hash_map, HashMap, HashSet}; pub fn mem2reg( header: &mut ModuleHeader, + types_global_values: &mut Vec, pointer_to_pointee: &HashMap, constants: &HashMap, func: &mut Function, @@ -26,6 +27,7 @@ pub fn mem2reg( let dominance_frontier = compute_dominance_frontier(&preds, &idom); insert_phis_all( header, + types_global_values, pointer_to_pointee, constants, &mut func.blocks, @@ -110,6 +112,7 @@ fn compute_dominance_frontier(preds: &[Vec], idom: &[usize]) -> Vec, pointer_to_pointee: &HashMap, constants: &HashMap, blocks: &mut [Block], @@ -132,6 +135,7 @@ fn insert_phis_all( let blocks_with_phi = insert_phis(blocks, &dominance_frontier, var_map); let mut renamer = Renamer { header, + types_global_values, blocks, blocks_with_phi, base_var_type, @@ -264,8 +268,38 @@ fn insert_phis( blocks_with_phi } +// These can't be part of the Renamer impl due to borrowck rules. +fn undef_for( + header: &mut ModuleHeader, + types_global_values: &mut Vec, + ty: Word, +) -> Word { + // TODO: This is horribly slow, fix this + let existing = types_global_values + .iter() + .find(|inst| inst.class.opcode == Op::Undef && inst.result_type.unwrap() == ty); + if let Some(existing) = existing { + return existing.result_id.unwrap(); + } + let inst_id = id(header); + types_global_values.push(Instruction::new(Op::Undef, Some(ty), Some(inst_id), vec![])); + inst_id +} +fn top_stack_or_undef( + header: &mut ModuleHeader, + types_global_values: &mut Vec, + stack: &[Word], + ty: Word, +) -> Word { + match stack.last() { + Some(&top) => top, + None => undef_for(header, types_global_values, ty), + } +} + struct Renamer<'a> { header: &'a mut ModuleHeader, + types_global_values: &'a mut Vec, blocks: &'a mut [Block], blocks_with_phi: HashSet, base_var_type: Word, @@ -284,7 +318,12 @@ impl Renamer<'_> { let existing_phi = self.blocks[block].instructions.iter_mut().find(|inst| { inst.class.opcode == Op::Phi && phi_defs.contains(&inst.result_id.unwrap()) }); - let top_def = *self.stack.last().unwrap(); + let top_def = top_stack_or_undef( + self.header, + self.types_global_values, + &self.stack, + self.base_var_type, + ); match existing_phi { None => { let new_id = id(self.header); @@ -344,7 +383,12 @@ impl Renamer<'_> { self.stack.push(val); } else { let new_id = id(self.header); - let prev_comp = *self.stack.last().unwrap(); + let prev_comp = top_stack_or_undef( + self.header, + self.types_global_values, + &self.stack, + self.base_var_type, + ); let mut operands = vec![Operand::IdRef(val), Operand::IdRef(prev_comp)]; operands .extend(var_info.indices.iter().copied().map(Operand::LiteralInt32)); @@ -361,7 +405,13 @@ impl Renamer<'_> { let ptr = inst.operands[0].id_ref_any().unwrap(); if let Some(var_info) = self.var_map.get(&ptr) { let loaded_val = inst.result_id.unwrap(); - let current_obj = *self.stack.last().unwrap(); + // TODO: Should this do something more sane if it's undef? + let current_obj = top_stack_or_undef( + self.header, + self.types_global_values, + &self.stack, + self.base_var_type, + ); if var_info.indices.is_empty() { *inst = Instruction::new(Op::Nop, None, None, vec![]); self.rewrite_rules.insert(loaded_val, current_obj); diff --git a/rustc_codegen_spirv/src/linker/mod.rs b/rustc_codegen_spirv/src/linker/mod.rs index 7507622f12..3c8acf01f0 100644 --- a/rustc_codegen_spirv/src/linker/mod.rs +++ b/rustc_codegen_spirv/src/linker/mod.rs @@ -184,6 +184,7 @@ pub fn link(sess: Option<&Session>, inputs: &mut [&mut Module], opts: &Options) // Note: mem2reg requires functions to be in RPO order (i.e. block_ordering_pass) mem2reg::mem2reg( output.header.as_mut().unwrap(), + &mut output.types_global_values, &pointer_to_pointee, &constants, func,