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

2018-02-26 Thread Jason Ekstrand 
On Mon, Feb 26, 2018 at 6:01 AM, Jose Maria Casanova Crespo <
jmcasan...@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
> 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
> , 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||
>

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 "||". :-)

Reviewed-by: Jason Ekstrand 


> + 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
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-02-26 Thread Jose Maria Casanova Crespo 
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 , 
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||
+ 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

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-02-26 Thread 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  >
> 
> On Fri, Feb 23, 2018 at 1:16 PM, Chema Casanova  > wrote:
> 
> On 23/02/18 20:09, Jason Ekstrand wrote:
> > On Fri, Feb 23, 2018 at 1:26 AM, Jose Maria Casanova Crespo
> > 
> >> 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 , 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
> >   

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

2018-02-23 Thread Jason Ekstrand 
Assuming the CTS is still happy with it after those changes,

Reviewed-by: Jason Ekstrand 

On Fri, Feb 23, 2018 at 1:16 PM, Chema Casanova 
wrote:

> On 23/02/18 20:09, Jason Ekstrand wrote:
> > On Fri, Feb 23, 2018 at 1:26 AM, Jose Maria Casanova Crespo
> > > 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 , 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
> >
> >
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-02-23 Thread Chema Casanova 
On 23/02/18 20:09, Jason Ekstrand wrote:
> On Fri, Feb 23, 2018 at 1:26 AM, Jose Maria Casanova Crespo
> > 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 , 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
> 
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-02-23 Thread Jason Ekstrand 
On Fri, Feb 23, 2018 at 1:26 AM, Jose Maria Casanova Crespo <
jmcasan...@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
> , 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.


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


> + /* 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-02-23 Thread Jose Maria Casanova Crespo 
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 , 
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);
 
   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);
+ /* 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev