[Mesa-dev] [PATCH v5 6/6] mesa/st: glsl_to_tgsi: tie in new temporary register merge approach

[Nicolai Hähnle]

On 25.06.2017 09:22, Gert Wollny wrote:
> This patch ties in the new temporary register lifetime estiamtion and

Tpyo: estimation


> rename mapping evaluation. In order to enable it, the evironment
> variable MESA_GLSL_TO_TGSI_NEW_MERGE must be set.

This make sense during development, but I'd say the goal here is to 
either merge this series unconditionally or not at all. Too many 
configuration options are poison.


> Performance to compare between the current and the new implementation
> were measured by running the shader-db in one thread; Numbers are in
> % of total run.
> 
> -----------------------------------------------------------
>                       old     new(qsort)   new(std::sort)
> 
> ------------------------ valgrind -------------------------
> merge                0.21       0.20          0.13
> estimate lifetime    0.03       0.05          0.05
> evaluate mapping  (incl=0.16)   0.12          0.06
> apply mapping        0.02       0.02          0.02
> 
> ---   perf (approximate because of statistic sampling) -------
> merge                0.24       0.20          0.14
> estimate lifetime    0.03       0.05          0.05
> evaluate mapping  (incl=0.16)   0.10          0.04
> apply mapping        0.05       0.05          0.05

Please provide total running times as well.

Apart from that, the patch looks good.

Cheers,
Nicolai


> ---
>   src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 29 ++++++++++++++++++++++++++---
>   1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> index 528fc4cc64..d4abee9d02 100644
> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> @@ -55,7 +55,7 @@
>   #include "st_glsl_types.h"
>   #include "st_nir.h"
>   #include "st_shader_cache.h"
> -#include "st_glsl_to_tgsi_private.h"
> +#include "st_glsl_to_tgsi_temprename.h"
>   
>   #include "util/hash_table.h"
>   #include <algorithm>
> @@ -322,6 +322,7 @@ public:
>   
>      void merge_two_dsts(void);
>      void merge_registers(void);
> +   void merge_registers_alternative(void);
>      void renumber_registers(void);
>   
>      void emit_block_mov(ir_assignment *ir, const struct glsl_type *type,
> @@ -5139,6 +5140,23 @@ glsl_to_tgsi_visitor::merge_two_dsts(void)
>      }
>   }
>   
> +void
> +glsl_to_tgsi_visitor::merge_registers_alternative(void)
> +{
> +   struct rename_reg_pair *renames =
> +         rzalloc_array(mem_ctx, struct rename_reg_pair, this->next_temp);
> +   struct lifetime *lifetimes =
> +         rzalloc_array(mem_ctx, struct lifetime, this->next_temp);
> +
> +   get_temp_registers_required_lifetimes(mem_ctx, &this->instructions,
> +                                         this->next_temp, lifetimes);
> +   get_temp_registers_remapping(mem_ctx, this->next_temp, lifetimes, renames);
> +   rename_temp_registers(renames);
> +
> +   ralloc_free(lifetimes);
> +   ralloc_free(renames);
> +}
> +
>   /* Merges temporary registers together where possible to reduce the number of
>    * registers needed to run a program.
>    *
> @@ -6603,8 +6621,13 @@ get_mesa_program_tgsi(struct gl_context *ctx,
>      while (v->eliminate_dead_code());
>   
>      v->merge_two_dsts();
> -   if (!skip_merge_registers)
> -      v->merge_registers();
> +   if (!skip_merge_registers) {
> +      if (getenv("MESA_GLSL_TO_TGSI_NEW_MERGE") != NULL)
> +         v->merge_registers_alternative();
> +      else
> +         v->merge_registers();
> +   }
> +
>      v->renumber_registers();
>   
>      /* Write the END instruction. */
> 


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.