[Mesa-dev] [PATCH 2/2] glsl: Relax ir_quadop_bitfield_insert type restrictions.

[Ilia Mirkin]

On Tue, Jan 5, 2016 at 8:34 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> While GLSL restricts bitfieldInsert's offset and bits parameters to
> be scalars, we shouldn't require this in the IR.
>
> In particular, opt_vectorize() tries to combine
>
> result.x = bitfieldInsert(src0.x, src1.x, src2.x, src3.x);
> result.y = bitfieldInsert(src0.y, src1.y, src2.y, src3.y);
> result.z = bitfieldInsert(src0.z, src1.z, src2.z, src3.z);
> result.w = bitfieldInsert(src0.w, src1.w, src2.w, src3.w);
>
> into a single ivec4 bitfieldInsert operation.  This currently breaks,
> because the last two types become ivec4 rather than int.  It seems
> perfectly reasonable to allow this.
>
> i965 lowers ir_quadop_bitfield_insert to ir_binop_bfm and ir_triop_bfi,
> which already lift these restrictions.
>
> (I debated about using is_integer() or base_type == GLSL_TYPE_INT here;
> I ended up relaxing it to allow either int/uint because ir_binop_bfm
> and ir_triop_bfi do that already.)
>
> Fixes assertion failures when compiling Shadow of Mordor vertex shaders
> on i965 in vec4 mode (where OptimizeForAOS enables opt_vectorize()).
>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> Cc: Matt Turner <mattst88 at gmail.com>
> Cc: mesa-stable at lists.freedesktop.org
> ---
>  src/glsl/ir_validate.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/glsl/ir_validate.cpp b/src/glsl/ir_validate.cpp
> index dcc079c..b5eaaff 100644
> --- a/src/glsl/ir_validate.cpp
> +++ b/src/glsl/ir_validate.cpp
> @@ -661,8 +661,8 @@ ir_validate::visit_leave(ir_expression *ir)
>     case ir_quadop_bitfield_insert:
>        assert(ir->operands[0]->type == ir->type);
>        assert(ir->operands[1]->type == ir->type);
> -      assert(ir->operands[2]->type == glsl_type::int_type);
> -      assert(ir->operands[3]->type == glsl_type::int_type);
> +      assert(ir->operands[2]->type->is_integer());
> +      assert(ir->operands[3]->type->is_integer());

I thought a bit more about this, and I think you need to check that
->vector_elements matches up, or is 1 -- you wouldn't want to allow
mixing a vec4 and vec2, for example. Not that anything would generate
that, but the point of these checks is to be strict about allowed
things.

As for int vs uint... meh. Don't care either way.

  -ilia