[Mesa-dev] [PATCH v2 2/2] i965/vec4_nir: Load constants as integers

[Antía Puentes]

Hi! Jason,

On lun, 2015-08-24 at 10:26 -0700, Jason Ekstrand wrote:
> Could you please debase this patch (probably rewrite)? I *think* it
> should fix https://bugs.freedesktop.org/show_bug.cgi?id=91716.  What
> were the shader-db results for it?

This V2 of the patch is already rebased/rewritten taking into account
the changes made in load_const to use fewer moves, sorry for not having
said it explicitly.

Because for loading the constants as integers we necessarily need to fix
the saturation errors, we would be having Piglit regressions in:
"shaders/glsl-clamp-vertex-color.shader_test" and
"arb_color_buffer_float-render" tests otherwise, I have applied both
patches to collect the shader-db results.
  
Comparing master (commit 529acab22a3e21e0ed0c5243675aec6c0ee27e8f) and
the version obtained by applying patches "i965/vec4: Fix saturation
errors when coalescing registers" and "i965/vec4_nir: Load constants as
integers", we have:

* Shader-db results for vec4 on i965 (Haswell):

total instructions in shared programs: 6402199 -> 6402200 (0.00%)
instructions in affected programs:     81 -> 82 (1.23%)
helped:                                0
HURT:                                  1
GAINED:                                0
LOST:                                  0

Printing the vec4 instructions generated by the hurt program, we can see
that this extra instruction is the result of reaching a program where
the register coalescing is not possible because saturation is required
and the instruction we want to coalesce into is not a MOV (if it were a
MOV, we could safely coalesce using the type of the final register,
instead of the type of the register to be removed as it is currently
done). I am detailing only the pseudo-code for the component x for
brevity, the rest of the components would be equivalent:

In the master branch, the following vec4 instructions:
  and vgrf8.0.x:D, vgrf7.xxxx:D, 1065353216U
  mov vgrf9.0.x:D, vgrf8.xxxx:D
  mov.sat m4:F, vgrf9.xyzw:F

are reduced by opt_register_coalesce to: 
  and.sat m4.x:D, vgrf7.xxxx:D, 1065353216U
making the saturation modifier a no-op (type D).

With the patches applied, opt_register_coalesce does not coalesce vgrf9,
we have an extra mov but the saturation is effective:
  and vgrf9.0.x:D, vgrf7.xxxx:D, 1065353216U
  mov.sat m4:F, vgrf9.xyzw:F

> On Aug 24, 2015 4:51 AM, "Antia Puentes" <apuentes at igalia.com> wrote:
>         Loads constants using integer as their register type, this is
>         done
>         for consistency with the FS backend.
>         ---
>          src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 4 ++--
>          1 file changed, 2 insertions(+), 2 deletions(-)
>         
>         diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>         b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>         
>         index 632e409..23b2fab 100644
>         --- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>         +++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>         @@ -456,7 +456,7 @@ void
>          vec4_visitor::nir_emit_load_const(nir_load_const_instr
>         *instr)
>          {
>             dst_reg reg = dst_reg(GRF, alloc.allocate(1));
>         -   reg.type =  BRW_REGISTER_TYPE_F;
>         +   reg.type =  BRW_REGISTER_TYPE_D;
>         
>         
>             unsigned remaining =
>         brw_writemask_for_size(instr->def.num_components);
>         
>         @@ -477,7 +477,7 @@
>         vec4_visitor::nir_emit_load_const(nir_load_const_instr *instr)
>                }
>         
>                reg.writemask = writemask;
>         -      emit(MOV(reg, src_reg(instr->value.f[i])));
>         +      emit(MOV(reg, src_reg(instr->value.i[i])));
>         
>         
>                remaining &= ~writemask;
>             }
>         --
>         2.1.0
>         
>         _______________________________________________
>         mesa-dev mailing list
>         mesa-dev at lists.freedesktop.org
>         http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>