El 23/02/18 a las 22:36, Jason Ekstrand escribió: > Assuming the CTS is still happy with it after those changes,
CTS was happy, but piglit has complained a lot. > > Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net > <mailto:ja...@jlekstrand.net>> > > On Fri, Feb 23, 2018 at 1:16 PM, Chema Casanova <jmcasan...@igalia.com > <mailto:jmcasan...@igalia.com>> wrote: > > On 23/02/18 20:09, Jason Ekstrand wrote: > > On Fri, Feb 23, 2018 at 1:26 AM, Jose Maria Casanova Crespo > > <jmcasan...@igalia.com <mailto:jmcasan...@igalia.com> > <mailto:jmcasan...@igalia.com <mailto:jmcasan...@igalia.com>>> wrote: > > > > The introduction of 16-bit types with VK_KHR_16bit_storages > implies that > > push constant offsets could be multiple of 2-bytes. Some > assertions are > > relaxed so offsets can be multiple of 4-bytes or multiple of > size of the > > base type. > > > > For 16-bit types, the push constant offset takes into account the > > internal offset in the 32-bit uniform bucket adding 2-bytes > when we > > access > > not 32-bit aligned elements. In all 32-bit aligned cases it just > > becomes 0. > > --- > > src/compiler/spirv/vtn_variables.c | 1 - > > src/intel/compiler/brw_fs_nir.cpp | 16 > +++++++++++----- > > src/intel/vulkan/anv_nir_lower_push_constants.c | 2 -- > > 3 files changed, 11 insertions(+), 8 deletions(-) > > > > diff --git a/src/compiler/spirv/vtn_variables.c > > b/src/compiler/spirv/vtn_variables.c > > index 81658afbd9..87236d0abd 100644 > > --- a/src/compiler/spirv/vtn_variables.c > > +++ b/src/compiler/spirv/vtn_variables.c > > @@ -760,7 +760,6 @@ _vtn_load_store_tail(struct vtn_builder *b, > > nir_intrinsic_op op, bool load, > > } > > > > if (op == nir_intrinsic_load_push_constant) { > > - vtn_assert(access_offset % 4 == 0); > > > > nir_intrinsic_set_base(instr, access_offset); > > nir_intrinsic_set_range(instr, access_size); > > diff --git a/src/intel/compiler/brw_fs_nir.cpp > > b/src/intel/compiler/brw_fs_nir.cpp > > index abf9098252..27611a21d0 100644 > > --- a/src/intel/compiler/brw_fs_nir.cpp > > +++ b/src/intel/compiler/brw_fs_nir.cpp > > @@ -3887,16 +3887,22 @@ fs_visitor::nir_emit_intrinsic(const > > fs_builder &bld, nir_intrinsic_instr *instr > > break; > > > > case nir_intrinsic_load_uniform: { > > - /* Offsets are in bytes but they should always be multiples > > of 4 */ > > - assert(instr->const_index[0] % 4 == 0); > > + /* Offsets are in bytes but they should always be > multiple of 4 > > + * or multiple of the size of the destination type. 2 > for 16-bits > > + * types. > > > > + */ > > + assert(instr->const_index[0] % 4 == 0 || > > + instr->const_index[0] % type_sz(dest.type) == 0); > > > > > > Doubles are required to be 8-byte aligned so we can just have the dest > > type size check. > > Changed locally. It seems that we can not guarantee that doubles are aligned with offset of 8-bytes, Without the % 4 == 0 several tests crash like: Test: piglit.spec.arb_gpu_shader_int64.execution.conversion.frag-conversion-explicit-uvec3-i64vec3 In this case we have a ivec3 uniform and then a i64vec3 whose offset becomes 12 at this assertion. > > > > > > fs_reg src(UNIFORM, instr->const_index[0] / 4, dest.type); > > > > nir_const_value *const_offset = > > nir_src_as_const_value(instr->src[0]); > > if (const_offset) { > > - /* Offsets are in bytes but they should always be > > multiples of 4 */ > > - assert(const_offset->u32[0] % 4 == 0); > > - src.offset = const_offset->u32[0]; > > + assert(const_offset->u32[0] % 4 == 0 || > > + const_offset->u32[0] % type_sz(dest.type) == 0); > > > > > > Same here. > > Changed locally. This assertion change didn't raise any regression. I'm sending v2 with just this change. > > + /* For 16-bit types we add the module of the > const_index[0] > > + * offset to access to not 32-bit aligned element */ > > + src.offset = const_offset->u32[0] + > instr->const_index[0] % 4; > > > > for (unsigned j = 0; j < instr->num_components; j++) { > > bld.MOV(offset(dest, bld, j), offset(src, bld, j)); > > diff --git a/src/intel/vulkan/anv_nir_lower_push_constants.c > > b/src/intel/vulkan/anv_nir_lower_push_constants.c > > index b66552825b..ad60d0c824 100644 > > --- a/src/intel/vulkan/anv_nir_lower_push_constants.c > > +++ b/src/intel/vulkan/anv_nir_lower_push_constants.c > > @@ -41,8 +41,6 @@ anv_nir_lower_push_constants(nir_shader *shader) > > if (intrin->intrinsic != > nir_intrinsic_load_push_constant) > > continue; > > > > - assert(intrin->const_index[0] % 4 == 0); > > - > > /* We just turn them into uniform loads */ > > intrin->intrinsic = nir_intrinsic_load_uniform; > > } > > -- > > 2.14.3 > > > > > > > > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
