[Mesa-dev] [PATCH 2/4] glsl: fix bitfield_insert use when doing ldexp lowering

[Ilia Mirkin]

On Fri, Apr 25, 2014 at 2:44 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Fri, Apr 25, 2014 at 2:38 PM, Matt Turner <mattst88 at gmail.com> wrote:
>> On Fri, Apr 25, 2014 at 10:41 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>> bitfield_insert expects glsl_type::int_type arguments, not uint vectors
>>>
>>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>> ---
>>>
>>> Identical to the patch I sent out last night. Without it ldexp lowering
>>> (without bfi lowering) causes assertion failures in the next mesa/st patch.
>>>
>>>  src/glsl/lower_instructions.cpp | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/glsl/lower_instructions.cpp b/src/glsl/lower_instructions.cpp
>>> index 01ea0f0..54a7a23 100644
>>> --- a/src/glsl/lower_instructions.cpp
>>> +++ b/src/glsl/lower_instructions.cpp
>>> @@ -360,7 +360,8 @@ lower_instructions_visitor::ldexp_to_arith(ir_expression *ir)
>>>     ir_constant *sign_mask = new(ir) ir_constant(0x80000000u, vec_elem);
>>>
>>>     ir_constant *exp_shift = new(ir) ir_constant(23u, vec_elem);
>>> -   ir_constant *exp_width = new(ir) ir_constant(8u, vec_elem);
>>> +   ir_constant *exp_shift_int = new(ir) ir_constant(23);
>>> +   ir_constant *exp_width = new(ir) ir_constant(8);
>>>
>>>     /* Temporary variables */
>>>     ir_variable *x = new(ir) ir_variable(ir->type, "x", ir_var_temporary);
>>> @@ -421,10 +422,9 @@ lower_instructions_visitor::ldexp_to_arith(ir_expression *ir)
>>>      *     floating-point type, the result is undefined."
>>>      */
>>>
>>> -   ir_constant *exp_shift_clone = exp_shift->clone(ir, NULL);
>>>     ir->operation = ir_unop_bitcast_i2f;
>>>     ir->operands[0] = bitfield_insert(bitcast_f2i(x), resulting_biased_exp,
>>> -                                     exp_shift_clone, exp_width);
>>> +                                     exp_shift_int, exp_width);
>>>     ir->operands[1] = NULL;
>>>
>>>     /* Don't generate new IR that would need to be lowered in an additional
>>> --
>>> 1.8.3.2
>>
>> Interesting. Yeah, this is wrong. I was wondering why I wasn't seeing
>> a problem, but it's because i965 lowers bitfield_insert and presumably
>> your driver doesn't.
>>
>> I can't reproduce the issue, but can't you just do this? If so, I'll
>> send a patch.
>
> I'll check. I assumed you couldn't because exp_shift was being used by
> something else and that it would complain with the non-vector
> argument. But I never tried it, will do and let you know in a bit.

Yup, that works too. I guess rshift() is happy taking a glsl_type::int
instead of a vector. (I didn't check if the bfi -> bfm lowering pass
is happy since I haven't added support for bfm in gallium, as there
doesn't seem to be a need.)

>
>>
>> diff --git a/src/glsl/lower_instructions.cpp b/src/glsl/lower_instructions.cpp
>> index 01ea0f0..49316d0 100644
>> --- a/src/glsl/lower_instructions.cpp
>> +++ b/src/glsl/lower_instructions.cpp
>> @@ -359,8 +359,8 @@
>> lower_instructions_visitor::ldexp_to_arith(ir_expression *ir)
>>
>>     ir_constant *sign_mask = new(ir) ir_constant(0x80000000u, vec_elem);
>>
>> -   ir_constant *exp_shift = new(ir) ir_constant(23u, vec_elem);
>> -   ir_constant *exp_width = new(ir) ir_constant(8u, vec_elem);
>> +   ir_constant *exp_shift = new(ir) ir_constant(23);
>> +   ir_constant *exp_width = new(ir) ir_constant(8);
>>
>>     /* Temporary variables */
>>     ir_variable *x = new(ir) ir_variable(ir->type, "x", ir_var_temporary);