[Mesa-dev] [PATCH 2/2] glsl: Handle bits=32 case in bitfieldInsert/bitfieldExtract.

[Roland Scheidegger]

Am 30.12.2015 um 21:26 schrieb Matt Turner:
> The OpenGL specifications for these functions say:
> 
>    The result will be undefined if <offset> or <bits> is negative, or if
>    the sum of <offset> and <bits> is greater than the number of bits
>    used to store the operand.
> 
> Therefore passing bits=32, offset=0 is legal and defined in GLSL.
> 
> But the earlier DX11/SM5 bfi/ibfe/ubfe opcodes are specified to accept a
> bitfield width ranging from 0-31. As such, Intel and AMD instructions
> read only the low 5 bits of the width operand, making them not compliant
> with the GLSL spec, so we have to special case the bits=32 case.
> 
> Checking that offset=0 is not necessary, since for any other value,
> <offset> + <bits> will be greater than 32, which is specified as
> generating an undefined result.
What about offset=32, bits=0, will that work?

> 
> Fixes:
>    ES31-CTS.shader_bitfield_operation.bitfieldInsert.uint_2
>    ES31-CTS.shader_bitfield_operation.bitfieldInsert.uvec4_3
>    ES31-CTS.shader_bitfield_operation.bitfieldExtract.uvec3_0
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92595
> ---
> Yuck. Suggestions welcome.
Do blob drivers do that correctly? Otherwise make it a spec bug?
(Albeit I can see from a theoretical point of view why those 0/32 cases
make sense so it probably really is intentional.)

Roland

> 
>  src/glsl/builtin_functions.cpp  | 6 +++++-
>  src/glsl/lower_instructions.cpp | 7 +++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/src/glsl/builtin_functions.cpp b/src/glsl/builtin_functions.cpp
> index 602852a..3d5de83 100644
> --- a/src/glsl/builtin_functions.cpp
> +++ b/src/glsl/builtin_functions.cpp
> @@ -4894,7 +4894,11 @@ builtin_builder::_bitfieldExtract(const glsl_type *type)
>     ir_variable *bits   = in_var(glsl_type::int_type, "bits");
>     MAKE_SIG(type, gpu_shader5_or_es31, 3, value, offset, bits);
>  
> -   body.emit(ret(expr(ir_triop_bitfield_extract, value, offset, bits)));
> +   ir_if *if_32 = new(mem_ctx) ir_if(greater(bits, imm(31)));
> +   if_32->then_instructions.push_tail(ret(rshift(value, offset)));
> +   if_32->else_instructions.push_tail(
> +      ret(expr(ir_triop_bitfield_extract, value, offset, bits)));
> +   body.emit(if_32);
>  
>     return sig;
>  }
> diff --git a/src/glsl/lower_instructions.cpp b/src/glsl/lower_instructions.cpp
> index 845cfff..8a425a8 100644
> --- a/src/glsl/lower_instructions.cpp
> +++ b/src/glsl/lower_instructions.cpp
> @@ -359,10 +359,9 @@ lower_instructions_visitor::bitfield_insert_to_bfm_bfi(ir_expression *ir)
>     ir_rvalue *base_expr = ir->operands[0];
>  
>     ir->operation = ir_triop_bfi;
> -   ir->operands[0] = new(ir) ir_expression(ir_binop_bfm,
> -                                           ir->type->get_base_type(),
> -                                           ir->operands[3],
> -                                           ir->operands[2]);
> +   ir->operands[0] = lshift(rshift(new(ir) ir_constant(~0u),
> +                                   sub(new(ir) ir_constant(32), ir->operands[3])),
> +                            ir->operands[2]);
>     /* ir->operands[1] is still the value to insert. */
>     ir->operands[2] = base_expr;
>     ir->operands[3] = NULL;
>