<div dir="ltr"><div>Assuming the CTS is still happy with it after those changes, </div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </div><div class="gmail_extra"> <div class="gmail_quote">On Fri, Feb 23, 2018 at 1:16 PM, Chema Casanova <<a href="mailto:jmcasanova@igalia.com" target="_blank">jmcasanova@igalia.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 23/02/18 20:09, Jason Ekstrand wrote: > On Fri, Feb 23, 2018 at 1:26 AM, Jose Maria Casanova Crespo <div><div class="h5">> <<a href="mailto:jmcasanova@igalia.com">jmcasanova@igalia.com</a> <mailto:<a href="mailto:jmcasanova@igalia.com">jmcasanova@igalia.com</a>>> 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. </div></div>Changed locally. > > > 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. <div class="HOEnZb"><div class="h5"> > + /* 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 > > </div></div></blockquote></div> </div>