[Mesa-dev] [PATCH 4/4] i965: Properly handle integer types in opt_vector_float().

[Matt Turner]

On Mon, Apr 18, 2016 at 7:20 AM, Iago Toral <itoral at igalia.com> wrote:
> On Sun, 2016-04-17 at 23:14 -0700, Kenneth Graunke wrote:
>> Previously, opt_vector_float() always interpreted MOV sources as
>> floating point, and always created a MOV with a F-type destination.
>>
>> This meant that we could mess up sequences of integer loads, such as:
>>
>>    mov vgrf6.0.x:D, 0D
>>    mov vgrf6.0.y:D, 1D
>>    mov vgrf6.0.z:D, 2D
>>    mov vgrf6.0.w:D, 3D
>>
>> Here, integer 0/1/2/3 become approximately 0.0f, so we generated:
>>
>>    mov vgrf6.0:F, [0F, 0F, 0F, 0F]
>>
>> which is clearly wrong.  We can properly handle this by converting
>> integer values to float (rather than bitcasting), and emitting a type
>> converting MOV:
>>
>>    mov vgrf6.0:D, [0F, 1F, 2F, 3F]
>>
>> To do this, see first see if the integer values (converted to float)
>> are representable.  If so, we use a D-type MOV.  If not, we then try
>> the floating point values and an F-type MOV.  We make zero not impose
>> type restrictions.  This is important because 0D would imply a D-type
>> MOV, but is often used in sequences such as MOV 0D, MOV 0x3f800000D,
>> where we want to use an F-type MOV.
>>
>> Fixes about 54 dEQP-GLES2 failures with the vec4 VS backend.  This
>> recently became visible due to changes in opt_vector_float() which
>> made it optimize more cases, but it was a pre-existing bug.
>>
>> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
>> ---
>>  src/mesa/drivers/dri/i965/brw_vec4.cpp | 24 ++++++++++++++++++++----
>>  1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp b/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> index 12c3c66..2bdcf1f 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> @@ -361,9 +361,11 @@ vec4_visitor::opt_vector_float()
>>     int inst_count = 0;
>>     vec4_instruction *imm_inst[4];
>>     unsigned writemask = 0;
>> +   enum brw_reg_type dest_type = BRW_REGISTER_TYPE_F;
>>
>>     foreach_block_and_inst_safe(block, vec4_instruction, inst, cfg) {
>>        int vf = -1;
>> +      enum brw_reg_type need_type;
>>
>>        /* Look for unconditional MOVs from an immediate with a partial
>>         * writemask.  Skip type-conversion MOVs other than integer 0,
>> @@ -375,14 +377,26 @@ vec4_visitor::opt_vector_float()
>>            inst->predicate == BRW_PREDICATE_NONE &&
>>            inst->dst.writemask != WRITEMASK_XYZW &&
>>            (inst->src[0].type == inst->dst.type || inst->src[0].d == 0)) {
>> -         vf = brw_float_to_vf(inst->src[0].f);
>> +         vf = brw_float_to_vf(inst->src[0].d);
>> +         need_type = BRW_REGISTER_TYPE_D;
>> +
>> +         if (vf == -1) {
>> +            vf = brw_float_to_vf(inst->src[0].f);
>> +            need_type = BRW_REGISTER_TYPE_F;
>> +         }
>
> If we are packing actual float values (not integers), doesn't this mean
> that we re-interpret them as integers and convert the re-interpreted
> integer value to float? If the result of that sequence of operations is
> representable it seems that we would just use a D-MOV from a float that
> no longer represents the original value, right?

I believe you're correct, but that there are no values for which this
could cause a problem.

Since the case we're trying to handle is loading, e.g., integer
<1,2,3,4> as a MOV dst:D, [1,2,3,4]VF it might be safer (if there are
in fact problematic values) and clearer to only attempt to reinterpret
the bits as as integer if the destination type is an integer type.