[Mesa-dev] [PATCH 6/7] spirv/i965/anv: Relax push constant offset assertions being 32-bit aligned

[Chema Casanova]

On 23/02/18 20:09, Jason Ekstrand wrote:
> On Fri, Feb 23, 2018 at 1:26 AM, Jose Maria Casanova Crespo
> <jmcasanova at igalia.com <mailto:jmcasanova at 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.

>  
> 
>            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.

>     +         /* 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
> 
>