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

[Iago Toral]

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?

Example:

.f = 5.27 (0x40a8a3d7)
.d = 1084793815 (0x40a8a3d7)

so we would do brw_float_to_vf(1084793815.0) instead of
brw_float_to_vf(5.27), which does not look right.

> +         /* Zero can be loaded as any type; don't impose a restriction. */
> +         if (inst->src[0].d == 0)
> +            need_type = dest_type;
>        }
>  
>        /* If this wasn't a MOV, or the value was non-representable, or
> -       * the destination register doesn't match, then this breaks our
> -       * sequence.  Combine anything we've accumulated so far.
> +       * the destination register doesn't match, or we have to switch
> +       * destination types, then this breaks our sequence.  Combine
> +       * anything we've accumulated so far.
>         */
>        if (vf == -1 ||
> +          dest_type != need_type ||
>            last_reg != inst->dst.nr ||
>            last_reg_offset != inst->dst.reg_offset ||
>            last_reg_file != inst->dst.file) {
> @@ -391,7 +405,7 @@ vec4_visitor::opt_vector_float()
>              unsigned vf;
>              memcpy(&vf, imm, sizeof(vf));
>              vec4_instruction *mov = MOV(imm_inst[0]->dst, brw_imm_vf(vf));
> -            mov->dst.type = BRW_REGISTER_TYPE_F;
> +            mov->dst.type = dest_type;
>              mov->dst.writemask = writemask;
>              inst->insert_before(block, mov);
>  
> @@ -405,6 +419,7 @@ vec4_visitor::opt_vector_float()
>           inst_count = 0;
>           last_reg = -1;
>           writemask = 0;
> +         dest_type = BRW_REGISTER_TYPE_F;
>  
>           for (int i = 0; i < 4; i++) {
>              imm[i] = 0;
> @@ -428,6 +443,7 @@ vec4_visitor::opt_vector_float()
>           last_reg = inst->dst.nr;
>           last_reg_offset = inst->dst.reg_offset;
>           last_reg_file = inst->dst.file;
> +         dest_type = need_type;
>        }
>     }
>