[v2,41/53] intel/compiler: assert that lower conversions produces valid strides

Submitted by Iago Toral Quiroga on Dec. 19, 2018, 11:51 a.m.

Details

Message ID 20181219115121.20815-42-itoral@igalia.com
State New
Headers show
Series "intel: VK_KHR_shader_float16_int8 implementation" ( rev: 3 ) in Mesa

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga Dec. 19, 2018, 11:51 a.m.
The hardware only has two bits to specify the horizontal stride, so the
maximum horizontal stride we can use is 4. The pass calculates strides
based on the sizes of the types involved, and for conversions between
64-bit and 8-bit types that can lead to strides of 8.

The compiler should make sure that such conversions are handled in two
steps to avoid that situation. If we fail to do this properly, the
generated assembly will be invalid and validation will fail, but
asserting here makes debugging easier.
---
 src/intel/compiler/brw_fs_lower_conversions.cpp | 7 +++++++
 1 file changed, 7 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/intel/compiler/brw_fs_lower_conversions.cpp b/src/intel/compiler/brw_fs_lower_conversions.cpp
index 145fb55f995..00781e824e8 100644
--- a/src/intel/compiler/brw_fs_lower_conversions.cpp
+++ b/src/intel/compiler/brw_fs_lower_conversions.cpp
@@ -90,6 +90,13 @@  fs_visitor::lower_conversions()
             fs_reg temp = ibld.vgrf(get_exec_type(inst));
             fs_reg strided_temp = subscript(temp, dst.type, 0);
 
+            /* Make sure we don't exceed hardware limits here. If we have code
+             * that hits this assertion it means that we need to split the
+             * instruction in two, using intermediary types (see for
+             * example nir_op_i2i8).
+             */
+            assert(strided_temp.stride <= 4);
+
             assert(inst->size_written == inst->dst.component_size(inst->exec_size));
             inst->dst = strided_temp;
             inst->saturate = false;

Comments

Subject reads a little odd, maybe just: "assert strides in conversion
lowering". The assert itself seems useful:

Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>

On Wed, Dec 19, 2018 at 12:51:09PM +0100, Iago Toral Quiroga wrote:
> The hardware only has two bits to specify the horizontal stride, so the
> maximum horizontal stride we can use is 4. The pass calculates strides
> based on the sizes of the types involved, and for conversions between
> 64-bit and 8-bit types that can lead to strides of 8.
> 
> The compiler should make sure that such conversions are handled in two
> steps to avoid that situation. If we fail to do this properly, the
> generated assembly will be invalid and validation will fail, but
> asserting here makes debugging easier.
> ---
>  src/intel/compiler/brw_fs_lower_conversions.cpp | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/intel/compiler/brw_fs_lower_conversions.cpp b/src/intel/compiler/brw_fs_lower_conversions.cpp
> index 145fb55f995..00781e824e8 100644
> --- a/src/intel/compiler/brw_fs_lower_conversions.cpp
> +++ b/src/intel/compiler/brw_fs_lower_conversions.cpp
> @@ -90,6 +90,13 @@ fs_visitor::lower_conversions()
>              fs_reg temp = ibld.vgrf(get_exec_type(inst));
>              fs_reg strided_temp = subscript(temp, dst.type, 0);
>  
> +            /* Make sure we don't exceed hardware limits here. If we have code
> +             * that hits this assertion it means that we need to split the
> +             * instruction in two, using intermediary types (see for
> +             * example nir_op_i2i8).
> +             */
> +            assert(strided_temp.stride <= 4);
> +
>              assert(inst->size_written == inst->dst.component_size(inst->exec_size));
>              inst->dst = strided_temp;
>              inst->saturate = false;
> -- 
> 2.17.1
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev