[Mesa-dev] [PATCH 48.1/59 v2] nir: Shift count for shift opcodes is always 32-bits

[Connor Abbott]

On Thu, Oct 27, 2016 at 10:06 PM, Ian Romanick <idr at freedesktop.org> wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
>
> Previously both sources were unsized.  This caused problems when the
> thing being shifted was 64-bit but the shift count was 32-bit.  The
> expectation in NIR is that all unsized sources (and destination) will
> ultimately have the same size.
>
> The changes in nir_opt_algebraic.py are to prevent errors like:
>
>  Failed to parse transformation:
> 03:12:25   (('extract_i8', 'a', 'b'), ('ishr', ('ishl', 'a', ('imul', ('isub', 3, 'b'), 8)), 24), 'options->lower_extract_byte')
> 03:12:25 Traceback (most recent call last):
> 03:12:25   File "/home/jenkins/workspace/Leeroy_2/repos/mesa/src/compiler/nir/nir_algebraic.py", line 610, in __init__
> 03:12:25     xform = SearchAndReplace(xform)
> 03:12:25   File "/home/jenkins/workspace/Leeroy_2/repos/mesa/src/compiler/nir/nir_algebraic.py", line 495, in __init__
> 03:12:25     BitSizeValidator(varset).validate(self.search, self.replace)
> 03:12:25   File "/home/jenkins/workspace/Leeroy_2/repos/mesa/src/compiler/nir/nir_algebraic.py", line 311, in validate
> 03:12:25     validate_dst_class = self._validate_bit_class_up(replace)
> 03:12:25   File "/home/jenkins/workspace/Leeroy_2/repos/mesa/src/compiler/nir/nir_algebraic.py", line 414, in _validate_bit_class_up
> 03:12:25     src_class = self._validate_bit_class_up(val.sources[i])
> 03:12:25   File "/home/jenkins/workspace/Leeroy_2/repos/mesa/src/compiler/nir/nir_algebraic.py", line 420, in _validate_bit_class_up
> 03:12:25     assert src_class == src_type_bits
> 03:12:25 AssertionError

It's pretty great that Jason wrote this validation pass, since
otherwise this would've been a rare and hard-to-diagnose failure
condition at runtime. Anyways, much better! This patch is

Reviewed-by: Connor Abbott <cwabbott0 at gmail.com>

The commit message on the next patch is out-of-date now, but other
than that it seems ok too. I think the only remaining issue is making
conversions use implicitly sized types.

>
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> Suggested-by: Connor Abbott <cwabbott0 at gmail.com>
> Cc: Connor Abbott <cwabbott0 at gmail.com>
> Cc: Jason Ekstrand <jason at jlekstrand.net>
> ---
>
> I will blame the cold medicine, but when I read Connor's statement,
> "legal for one input to be explicitly sized while the other is unsized"
> I parsed it more like "legal for one input to be one size while the
> other is another sized".  The latter is quite a bit more general than
> the former.  Oops.

It's fine, we all make silly mistakes sometimes :)

>
>  src/compiler/nir/nir_opcodes.py       | 6 +++---
>  src/compiler/nir/nir_opt_algebraic.py | 8 ++++----
>  2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/src/compiler/nir/nir_opcodes.py b/src/compiler/nir/nir_opcodes.py
> index 1825dc3..2af146a 100644
> --- a/src/compiler/nir/nir_opcodes.py
> +++ b/src/compiler/nir/nir_opcodes.py
> @@ -497,9 +497,9 @@ binop("seq", tfloat32, commutative, "(src0 == src1) ? 1.0f : 0.0f") # Set on Equ
>  binop("sne", tfloat32, commutative, "(src0 != src1) ? 1.0f : 0.0f") # Set on Not Equal
>
>
> -binop("ishl", tint, "", "src0 << src1")
> -binop("ishr", tint, "", "src0 >> src1")
> -binop("ushr", tuint, "", "src0 >> src1")
> +opcode("ishl", 0, tint, [0, 0], [tint, tuint32], "", "src0 << src1")
> +opcode("ishr", 0, tint, [0, 0], [tint, tuint32], "", "src0 >> src1")
> +opcode("ushr", 0, tuint, [0, 0], [tuint, tuint32], "", "src0 >> src1")
>
>  # bitwise logic operators
>  #
> diff --git a/src/compiler/nir/nir_opt_algebraic.py b/src/compiler/nir/nir_opt_algebraic.py
> index 82d92f4..5e384d9 100644
> --- a/src/compiler/nir/nir_opt_algebraic.py
> +++ b/src/compiler/nir/nir_opt_algebraic.py
> @@ -341,19 +341,19 @@ optimizations = [
>                ('ubfe', 'value', 'offset', 'bits')),
>      'options->lower_bitfield_extract'),
>
> -   (('extract_i8', a, b),
> +   (('extract_i8', a, 'b at 32'),
>      ('ishr', ('ishl', a, ('imul', ('isub', 3, b), 8)), 24),
>      'options->lower_extract_byte'),
>
> -   (('extract_u8', a, b),
> +   (('extract_u8', a, 'b at 32'),
>      ('iand', ('ushr', a, ('imul', b, 8)), 0xff),
>      'options->lower_extract_byte'),
>
> -   (('extract_i16', a, b),
> +   (('extract_i16', a, 'b at 32'),
>      ('ishr', ('ishl', a, ('imul', ('isub', 1, b), 16)), 16),
>      'options->lower_extract_word'),
>
> -   (('extract_u16', a, b),
> +   (('extract_u16', a, 'b at 32'),
>      ('iand', ('ushr', a, ('imul', b, 16)), 0xffff),
>      'options->lower_extract_word'),
>
> --
> 2.5.5
>