IR Validation: validate no identical types with different IDs This change walks through the IR.meta.types vector, and checks that there is no duplicated items in this vector. In order to use the HashSet, we need to implement Eq, PartialEq, and Hash traits for the Type enum. Some transformation will mark some Type variables as DeadCodeEliminated, these Type variables won't be included in final IR, so the type uniqueness check will skip them. For struct type, In GLSL ES Spec 3.2, section 4.1.8: https://registry.khronos.org/OpenGL/specs/es/3.2/GLSL_ES_Specification_3.20.html#structures It says that for struct type, "name becomes the user-defined type". However, for struct specifier declared as follows: ``` struct { int field; }s; ``` The GLSL parser code will use empty string for the struct type name. This means if the GLSL shader code declares 2 struct types: ``` struct { int field; }s1; struct { int field; }s2; ``` In IR, both will ends up with the same Type::Struct(Name, Vec<Field>, StructSpecialization). We can't tell they are different struct types from the ir::Type enum itself. Another problem is the interface blocks can end up with the same Type::Struct(Name, Vec<Field>, StructSpecialization). To solve the problem, we treat two Struct Types as different types if they both have empty name, or if they are interface blocks. This is a valid fallback, as SpirV doesn't enforce type uniqueness on struct in general: https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#TypesVariables: "two different aggregate type <id>s are allowed to have identical declarations and decorations, and will still be two different types." Bug: angleproject:349994211 Change-Id: I81df74607825bf7ceddc96c4374898a05385c1a0 Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/7689832 Commit-Queue: Yuxin Hu <yuxinhu@google.com> Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
diff --git a/src/compiler/translator/ir/src/ir.rs b/src/compiler/translator/ir/src/ir.rs index 55d3c43..0d9808f 100644 --- a/src/compiler/translator/ir/src/ir.rs +++ b/src/compiler/translator/ir/src/ir.rs
@@ -6,6 +6,7 @@ use super::instruction; use std::collections::{HashMap, HashSet}; +use std::hash::{Hash, Hasher}; // Strong types for ids that refer to constants, registers, variables, types etc. They are used to // look information up in different tables. In all cases, 0 means no applicable ID. @@ -1466,7 +1467,7 @@ } // Where a name came from. This affects how it is output. -#[derive(Copy, Clone, PartialEq)] +#[derive(Copy, Clone, PartialEq, Eq, Hash)] #[cfg_attr(debug_assertions, derive(Debug))] pub enum NameSource { // A name in the shader itself, which corresponds to an interface variable (input, output, @@ -1483,7 +1484,7 @@ } // A name associated with a variable, struct, struct field etc. -#[derive(Copy, Clone)] +#[derive(Copy, Clone, PartialEq, Eq, Hash)] #[cfg_attr(debug_assertions, derive(Debug))] pub struct Name { // This is a slice into the shader source, a static name, or otherwise an empty string. Either @@ -2026,6 +2027,94 @@ DeadCodeEliminated, } +impl PartialEq for Type { + fn eq(&self, other: &Self) -> bool { + match (self, other) { + (Type::Scalar(basic_type_1), Type::Scalar(basic_type_2)) => { + basic_type_1 == basic_type_2 + } + ( + Type::Image(basic_image_type_1, image_type_1), + Type::Image(basic_image_type_2, image_type_2), + ) => basic_image_type_1 == basic_image_type_2 && image_type_1 == image_type_2, + // https://registry.khronos.org/OpenGL/specs/es/3.2/GLSL_ES_Specification_3.20.html#structures + // Two structure types are the same if they have the same name + // However, the GLSL parser code will assign an empty string for the struct name if it + // is declared in the following way: struct + // { + // int field; + // }s; + // This means if the GLSL shader code declares 2 struct types: + // struct + // { + // int field; + // }s1; + // + // struct + // { + // int field; + // }s2; + // In IR, both will ends up with the same Struct Name. + // In this case, we should treat them as different types even if the struct Name are + // equal. + (Type::Struct(name1, _, _), Type::Struct(name2, _, _)) => name1 == name2, + ( + Type::Vector(scalar_type_id_1, vector_size_1), + Type::Vector(scalar_type_id_2, vector_size_2), + ) => scalar_type_id_1 == scalar_type_id_2 && vector_size_1 == vector_size_2, + ( + Type::Matrix(vector_type_id_1, matrix_size_1), + Type::Matrix(vector_type_id_2, matrix_size_2), + ) => vector_type_id_1 == vector_type_id_2 && matrix_size_1 == matrix_size_2, + ( + Type::Array(element_type_id_1, array_size_1), + Type::Array(element_type_id_2, array_size_2), + ) => element_type_id_1 == element_type_id_2 && array_size_1 == array_size_2, + (Type::UnsizedArray(element_type_id_1), Type::UnsizedArray(element_type_id_2)) => { + element_type_id_1 == element_type_id_2 + } + (Type::Pointer(pointed_type_id_1), Type::Pointer(pointed_type_id_2)) => { + pointed_type_id_1 == pointed_type_id_2 + } + (Type::DeadCodeEliminated, Type::DeadCodeEliminated) => true, + _ => false, + } + } +} + +impl Eq for Type {} + +impl Hash for Type { + fn hash<H: Hasher>(&self, state: &mut H) { + core::mem::discriminant(self).hash(state); + match self { + Type::Scalar(basic_type) => basic_type.hash(state), + Type::Image(image_basic_type, image_type) => { + image_basic_type.hash(state); + image_type.hash(state); + } + // https://registry.khronos.org/OpenGL/specs/es/3.2/GLSL_ES_Specification_3.20.html#structures + // Two structure types are the same if they have the same name + Type::Struct(name, _, _) => name.hash(state), + Type::Vector(scalar_type_id, vector_size) => { + scalar_type_id.hash(state); + vector_size.hash(state); + } + Type::Matrix(vector_type_id, matrix_size) => { + vector_type_id.hash(state); + matrix_size.hash(state); + } + Type::Array(element_type_id, array_size) => { + element_type_id.hash(state); + array_size.hash(state); + } + Type::UnsizedArray(element_type_id) => element_type_id.hash(state), + Type::Pointer(pointed_type_id) => pointed_type_id.hash(state), + Type::DeadCodeEliminated => {} + } + } +} + impl Type { pub fn new_void() -> Type { Type::Scalar(BasicType::Void) @@ -2101,6 +2190,18 @@ pub fn is_struct(&self) -> bool { matches!(self, Type::Struct(..)) } + + pub fn is_struct_with_empty_name(&self) -> bool { + match self { + Type::Struct(struct_name, _, _) => struct_name.name.is_empty(), + _ => false, + } + } + + pub fn is_struct_interface_block(&self) -> bool { + matches!(self, Type::Struct(_, _, StructSpecialization::InterfaceBlock)) + } + fn is_struct_containing_samplers_helper(&self, ir_meta: &IRMeta) -> bool { // The parser puts samplers at the end of the struct, so check the fields from the back for // any sampler or struct that contains samplers. Samplers in struct are only valid in ESSL
diff --git a/src/compiler/translator/ir/src/validator.rs b/src/compiler/translator/ir/src/validator.rs index 53b831e..269b8fa 100644 --- a/src/compiler/translator/ir/src/validator.rs +++ b/src/compiler/translator/ir/src/validator.rs
@@ -20,9 +20,9 @@ // output: validate_merge_block_with_input() // - For block that has a merge block with an input, the branch instruction must be If, and the // block must contains block1: validate_merge_block_with_input() +// - No identical types with different IDs: validate_no_identical_types_with_different_ids() // TODO(http://anglebug.com/349994211): to validate: -// - No identical types with different IDs. // - Referenced IDs are not dead-code-eliminated: Checking against max_*_count can be paird with a // look up and checking that !is_dead_code_eliminated // - If there's a cached "has side effect", that it's correct. @@ -254,6 +254,7 @@ self.validate_all_ids_are_present(); self.validate_all_variables_are_declared_in_scope(); self.validate_all_registers_are_declared_in_scope(); + self.validate_no_identical_types_with_different_ids(); self.validate_no_dead_code(); self.validate_all_branch_instructions_have_valid_target(); self.validate_merge_block_with_input(); @@ -932,6 +933,66 @@ registers_declared_map.remove_scope(); } + fn validate_no_identical_types_with_different_ids(&self) { + let mut seen_types: HashSet<&ir::Type> = HashSet::new(); + for ir_type in self.ir.meta.all_types() { + // Some Type will be marked as dead code eliminated during transform, they won't be + // included in final IR, skip them. + if ir_type.is_dead_code_eliminated() { + continue; + } + // https://registry.khronos.org/OpenGL/specs/es/3.2/GLSL_ES_Specification_3.20.html#structures + // Two structure types are the same if they have the same name + // However, for struct specifier declared as follows: + // struct + // { + // int field; + // }s; + // The GLSL parser code will use empty string for the struct type name. + // For the following struct declarations: + // struct + // { + // int field; + // }s1; + // struct + // { + // int field; + // }s2; + // In IR, we end up with two same ir::Type::Struct(name, fields, struct_specifier), + // where as we should treat them as different types. + // Skip the uniqueness check for struct with empty name for now. + if ir_type.is_struct_with_empty_name() { + continue; + } + + // For interface block, right now for the following interface block declarations + // in Vertex + // { + // ivec4 iv; + // vec4 fv; + // } inVertex[]; + // out Vertex + // { + // ivec4 iv; + // vec4 fv; + // } outVertex[]; + // They end up with the same ir::Type::Struct(name, fields, struct_specifier), but they + // should be treated as different types. + // Similar problem arises where the gl_PerVertex is redeclared in geometry shader, + // IR ends with with two same ir::Type::Struct(name, fields, struct_specifier). + // Skip the uniqueness check for interface block struct for now. + if ir_type.is_struct_interface_block() { + continue; + } + if !seen_types.insert(ir_type) { + self.on_error(format_args!( + "Identical type {:?} found with different IDs", + ir_type + )); + } + } + } + fn validate_all_branch_instructions_have_valid_target(&self) { let mut block_meta_data_tracker = Vec::new(); for entry in &self.ir.function_entries {