<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Feb 26, 2018 at 6:01 AM, Jose Maria Casanova Crespo <<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">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 updated so offsets should be just multiple of size of the base type but in some cases we can not assume it as doubles aren't aligned to 8 bytes in some cases. 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. v2: Assert offsets to be aligned to the dest type size. (Jason Ekstrand) --- src/compiler/spirv/vtn_variables.c | 2 -- src/intel/compiler/brw_fs_nir.cpp | 15 ++++++++++----- src/intel/vulkan/anv_nir_lower_push_constants.c | 2 -- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index 105b33a5671..7e8a090adde 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -753,8 +753,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 03d0bba24a8..6f721ac3a43 100644 --- a/src/intel/compiler/brw_fs_nir.cpp +++ b/src/intel/compiler/brw_fs_nir.cpp @@ -3887,16 +3887,21 @@ 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 aligned to + * the type size + */ + assert(instr->const_index[0] % 4 == 0|| </blockquote><div> </div><div>Hrm... I was going to say you don't need the first half of the assert but then I remembered that NIR may place doubles with bad alignments because we really don't care that much if they're getting pulled. In any case, you need a space before "||". :-) </div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + instr->const_index[0] % type_sz(dest.type) == 0); 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] % type_sz(dest.type) == 0); + /* 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 b66552825b9..ad60d0c824e 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 </blockquote></div> </div></div>