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

[Jason Ekstrand]

Assuming the CTS is still happy with it after those changes,

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>

On Fri, Feb 23, 2018 at 1:16 PM, Chema Casanova <jmcasanova at 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
> > <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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180223/f34210d3/attachment.html>