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

[Chema Casanova]

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 <jason at jlekstrand.net
> <mailto:jason at jlekstrand.net>>
> 
> On Fri, Feb 23, 2018 at 1:16 PM, Chema Casanova <jmcasanova at igalia.com
> <mailto: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>
>     <mailto: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.


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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>