[Mesa-dev] [PATCH 06/15] nir/algebraic: Improve error messages for replace expressions

[Timothy Arceri]

I'm no python expect but for what its worth patches 1-6 are:

Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>

On 9/11/18 2:45 pm, Jason Ekstrand wrote:
> In bf441d22a7917f38c, I wrote a bunch of descriptive asserts for various
> bit size checks in the validation of search expressions.  This commit
> improves a few of those and adds descriptive asserts for replace
> expressions as well.  We also rework _validate_bit_class_down so that it
> can properly handle the case where any bit size may be consumed but we
> can infer the correct size from something deeper in the tree.
> ---
>   src/compiler/nir/nir_algebraic.py | 75 ++++++++++++++++++++++++-------
>   1 file changed, 60 insertions(+), 15 deletions(-)
> 
> diff --git a/src/compiler/nir/nir_algebraic.py b/src/compiler/nir/nir_algebraic.py
> index 474d913e722..f9ee637830c 100644
> --- a/src/compiler/nir/nir_algebraic.py
> +++ b/src/compiler/nir/nir_algebraic.py
> @@ -423,7 +423,7 @@ class BitSizeValidator(object):
>            else:
>               if val.common_size != 0:
>                  assert val.bit_size == 0 or val.bit_size == val.common_size, \
> -                      'Variable width expression musr be {0}-bit based on ' \
> +                      'Variable width expression must be {0}-bit based on ' \
>                         'the sources but a {1}-bit result was requested: {2}' \
>                         .format(val.common_size, val.bit_size, str(val))
>               else:
> @@ -445,14 +445,16 @@ class BitSizeValidator(object):
>            if dst_type_bits != 0:
>               assert bit_class == 0 or bit_class == dst_type_bits, \
>                      'nir_op_{0} produces a {1}-bit result but the parent ' \
> -                   'expression wants a {2}-bit value' \
> -                   .format(val.opcode, dst_type_bits, bit_class)
> +                   'expression wants a {2} value' \
> +                   .format(val.opcode, dst_type_bits,
> +                           self._bit_class_to_str(bit_class))
>            else:
>               assert val.common_size == 0 or val.common_size == bit_class, \
>                      'Variable-width expression produces a {0}-bit result ' \
>                      'based on the source widths but the parent expression ' \
> -                   'wants a {1}-bit value: {2}' \
> -                   .format(val.common_size, bit_class, str(val))
> +                   'wants a {1} value: {2}' \
> +                   .format(val.common_size,
> +                           self._bit_class_to_str(bit_class), str(val))
>               val.common_size = bit_class
>   
>            if val.common_size:
> @@ -476,11 +478,16 @@ class BitSizeValidator(object):
>         elif isinstance(val, Variable):
>            var_class = self._get_var_bit_class(val)
>            # By the time we get to validation, every variable should have a class
> -         assert var_class != 0
> +         assert var_class != 0, \
> +                'Variable {} appears in the replace expression but not in ' \
> +                'the search expression'.format(str(val))
>   
>            # If we have an explicit size provided by the user, the variable
>            # *must* exactly match the search.  It cannot be implicitly sized
>            # because otherwise we could end up with a conflict at runtime.
> +         # This is guaranteed by the fact that we explicitly initialize bit
> +         # classes; the assert should never trigger even for malformed
> +         # expressions.
>            assert val.bit_size == 0 or val.bit_size == var_class
>   
>            return var_class
> @@ -495,18 +502,37 @@ class BitSizeValidator(object):
>   
>               src_type_bits = type_bits(nir_op.input_types[i])
>               if src_type_bits != 0:
> -               assert src_class == src_type_bits
> +               assert src_class == src_type_bits, \
> +                      'Source {} of nir_op_{} must be a {}-bit value but ' \
> +                      'the constructed value would be {}: {}' \
> +                      .format(i, val.opcode, src_type_bits,
> +                              self._bit_class_to_str(src_class), str(val))
>               else:
> -               assert val.common_class == 0 or src_class == val.common_class
> +               assert val.common_class == 0 or src_class == val.common_class, \
> +                      'Source {} of nir_op_{} must be a {} value based ' \
> +                      'on other sources but the constructed value would be ' \
> +                      '{}: {}' \
> +                      .format(i, val.opcode,
> +                              self._bit_class_to_str(val.common_class),
> +                              self._bit_class_to_str(src_class), str(val))
>                  val.common_class = src_class
>   
>            dst_type_bits = type_bits(nir_op.output_type)
>            if dst_type_bits != 0:
> -            assert val.bit_size == 0 or val.bit_size == dst_type_bits
> +            assert val.bit_size == 0 or val.bit_size == dst_type_bits, \
> +                   'Result of nir_op_{} must be a {}-bit value but the ' \
> +                   'expression explicitly requests a {}-bit value' \
> +                   .format(val.opcode, dst_type_bits, val.bit_size)
>               return dst_type_bits
>            else:
>               if val.common_class != 0:
> -               assert val.bit_size == 0 or val.bit_size == val.common_class
> +               assert val.bit_size == 0 or val.bit_size == val.common_class, \
> +                      'Result of nir_op_{} must be a {} value based ' \
> +                      'on the sources but the expression explicitly ' \
> +                      'requests a {}-bit value: {}' \
> +                      .format(val.opcode,
> +                              self._bit_class_to_str(val.common_class),
> +                              val.bit_size, str(val))
>               else:
>                  val.common_class = val.bit_size
>               return val.common_class
> @@ -514,21 +540,40 @@ class BitSizeValidator(object):
>      def _validate_bit_class_down(self, val, bit_class):
>         # At this point, everything *must* have a bit class.  Otherwise, we have
>         # a value we don't know how to define.
> -      assert bit_class != 0
> +      assert bit_class != 0, \
> +             'Value cannot be constructed because no bit-size is implied '\
> +             '{}'.format(str(val))
>   
>         if isinstance(val, Constant):
> -         assert val.bit_size == 0 or val.bit_size == bit_class
> +         assert val.bit_size == 0 or val.bit_size == bit_class, \
> +                'Constant value {} explicitly requests being {}-bit but ' \
> +                'must be {} thanks to its consumer' \
> +                .format(str(val), val.bit_size,
> +                        self._bit_class_to_str(bit_class))
>   
>         elif isinstance(val, Variable):
> -         assert val.bit_size == 0 or val.bit_size == bit_class
> +         assert val.bit_size == 0 or val.bit_size == bit_class, \
> +                'Variable {} explicitly only matches {}-bit values but ' \
> +                'must be {} thanks to its consumer' \
> +                .format(str(val), val.bit_size,
> +                        self._bit_class_to_str(bit_class))
>   
>         elif isinstance(val, Expression):
>            nir_op = opcodes[val.opcode]
>            dst_type_bits = type_bits(nir_op.output_type)
>            if dst_type_bits != 0:
> -            assert bit_class == dst_type_bits
> +            assert bit_class == dst_type_bits, \
> +                   'Result of nir_op_{} must be a {}-bit value but the ' \
> +                   'consumer requires a {} value: {}' \
> +                   .format(val.opcode, dst_type_bits,
> +                           self._bit_class_to_str(bit_class), str(val))
>            else:
> -            assert val.common_class == 0 or val.common_class == bit_class
> +            assert val.common_class == 0 or val.common_class == bit_class, \
> +                   'Result of nir_op_{} must be a {} value but based on ' \
> +                   'the sources but the consumer requires a {} value: {}' \
> +                   .format(val.opcode,
> +                           self._bit_class_to_str(val.common_class),
> +                           self._bit_class_to_str(bit_class), str(val))
>               val.common_class = bit_class
>   
>            for i in range(nir_op.num_inputs):
>