[Mesa-dev] [PATCH] glsl_to_tgsi: allocate and enlarge arrays for temporaries on demand

[Ilia Mirkin]

On Sun, Aug 24, 2014 at 7:44 AM, Marek Olšák <maraeo at gmail.com> wrote:
> From: Marek Olšák <marek.olsak at amd.com>
>
> This fixes crashes if the number of temporaries is greater than 4096.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=66184
> Cc: 10.2 10.3 mesa-stable at lists.freedesktop.org
> ---
>  src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 45 ++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> index 84bdc4f..9beecf8 100644
> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> @@ -75,14 +75,6 @@ extern "C" {
>                             (1 << PROGRAM_UNIFORM))
>
>  /**
> - * Maximum number of temporary registers.
> - *
> - * It is too big for stack allocated arrays -- it will cause stack overflow on
> - * Windows and likely Mac OS X.
> - */
> -#define MAX_TEMPS         4096
> -
> -/**
>   * Maximum number of arrays
>   */
>  #define MAX_ARRAYS        256
> @@ -3301,14 +3293,10 @@ get_src_arg_mask(st_dst_reg dst, st_src_reg src)
>  void
>  glsl_to_tgsi_visitor::simplify_cmp(void)
>  {
> -   unsigned *tempWrites;
> +   int tempWritesSize = 0;
> +   unsigned *tempWrites = NULL;
>     unsigned outputWrites[MAX_PROGRAM_OUTPUTS];
>
> -   tempWrites = new unsigned[MAX_TEMPS];
> -   if (!tempWrites) {
> -      return;
> -   }
> -   memset(tempWrites, 0, sizeof(unsigned) * MAX_TEMPS);
>     memset(outputWrites, 0, sizeof(outputWrites));
>
>     foreach_in_list(glsl_to_tgsi_instruction, inst, &this->instructions) {
> @@ -3330,7 +3318,16 @@ glsl_to_tgsi_visitor::simplify_cmp(void)
>           prevWriteMask = outputWrites[inst->dst.index];
>           outputWrites[inst->dst.index] |= inst->dst.writemask;
>        } else if (inst->dst.file == PROGRAM_TEMPORARY) {
> -         assert(inst->dst.index < MAX_TEMPS);
> +         if (inst->dst.index >= tempWritesSize) {
> +            const int inc = 4096;
> +
> +            tempWrites = (unsigned*)
> +                         realloc(tempWrites,
> +                                 (tempWritesSize + inc) * sizeof(unsigned));

This can fail, the old code dealt with new returning NULL. Not sure
what the general policy for that is in mesa, but I have seen a lot of
allocation checks. Probably makes sense here too.

> +            memset(tempWrites + tempWritesSize, 0, inc * sizeof(unsigned));
> +            tempWritesSize += inc;
> +         }
> +
>           prevWriteMask = tempWrites[inst->dst.index];
>           tempWrites[inst->dst.index] |= inst->dst.writemask;
>        } else
> @@ -3349,7 +3346,7 @@ glsl_to_tgsi_visitor::simplify_cmp(void)
>        }
>     }
>
> -   delete [] tempWrites;
> +   free(tempWrites);
>  }
>
>  /* Replaces all references to a temporary register index with another index. */
> @@ -4158,7 +4155,9 @@ struct label {
>  struct st_translate {
>     struct ureg_program *ureg;
>
> -   struct ureg_dst temps[MAX_TEMPS];
> +   unsigned temps_size;
> +   struct ureg_dst *temps;
> +
>     struct ureg_dst arrays[MAX_ARRAYS];
>     struct ureg_src *constants;
>     struct ureg_src *immediates;
> @@ -4299,7 +4298,16 @@ dst_register(struct st_translate *t,
>        return ureg_dst_undef();
>
>     case PROGRAM_TEMPORARY:
> -      assert(index < Elements(t->temps));
> +      /* Allocate space for temporaries on demand. */
> +      if (index >= t->temps_size) {
> +         const int inc = 4096;
> +
> +         t->temps = (struct ureg_dst*)
> +                    realloc(t->temps,
> +                            (t->temps_size + inc) * sizeof(struct ureg_dst));

Check alloc?

With that fixed,

Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>

BTW, should this in any way be related to PIPE_SHADER_CAP_MAX_TEMPS?
Looking at what drivers set this to, it's fairly small values. e.g.
nvc0 returns 128. I guess there's limited spilling space. OTOH, it can
do a *lot* better than TGSI at storing stuff due to optimizations,
register reuse, etc.

> +         memset(t->temps + t->temps_size, 0, inc * sizeof(struct ureg_dst));
> +         t->temps_size += inc;
> +      }
>
>        if (ureg_dst_is_undef(t->temps[index]))
>           t->temps[index] = ureg_DECL_local_temporary(t->ureg);
> @@ -5158,6 +5166,7 @@ st_translate_program(
>
>  out:
>     if (t) {
> +      free(t->temps);
>        free(t->insn);
>        free(t->labels);
>        free(t->constants);
> --
> 1.9.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev