[Mesa-dev] [PATCH v4 00/15] TGSI: improved live range tracking, also including arrays

[Gert Wollny]

Hello Benedikt, 

Am Mittwoch, den 06.06.2018, 21:58 +0200 schrieb Benedikt Schemmer:
> 
> The arrays-of-arrays piglit shaders I sent you still crash though
> Gert.
Can you post a backtrace? 

> 
> 
> -   if (get_temp_registers_required_live_ranges(   
> reg_live_ranges, &this->instructions, 
> +//   if (get_temp_registers_required_live_ranges(reg_live_ranges,
> &this->instructions,
This IF is actually only a fail-save for the case that in the future
someone would add subroutines to TGSI shaders that are not currently
handled in the live range tracker, so commenting this out should do
nothing.  

...
> 
> -      this->next_array =  merge_arrays(this->next_array, this-
> >array_sizes,
> -                                       &this->instructions,
> arr_live_ranges);
> +//      this->next_array =  merge_arrays(this->next_array, this-
> >array_sizes,
> +//                                       &this->instructions,
> arr_live_ranges);
This is actually the whole point of this series (with expection of the
first three patches). merge_arrays does two things: doing a live-range
based merge of arrays and interleave arrays that do not use all
components but are live at the same time. I could imagine that this
latter optimization could hurt LLVM because it could make it more
difficult to optimize vector operations. This reminds me that I should
probably run some benchmarks with llvmpipe. 


[...]
> 
> +/* these magic numbers are determined by looking at the results of
> shader-db */
> +bool should_merge (int distance)
> +{
> +   switch (distance) {
> +       case 12 ... 126: //lower bound interfering with llvm?, upper
> bound here
> +       case 244 ... 768: // and lower bound here determined by one
> regressing tombraider shader
> +// nothing to see here
> +       case 2432 ... 2496: // purely empiricily determined
> +       case 2497 ... 2623: // Deus Ex
> +       case 2624 ... 2688:
> +//     case 2689 ... 2943: // causes regressions in ubershaders
> +       case 2944 ... 3072: // above isnt used
> +          return true;
> +       default:
> +          return false;
> +   }
> +}
I would guess that such a table would have to be updated with every new
shader coming out. 

Best, 
Gert

>  /* This functions evaluates the register merges by using a binary
>   * search to find suitable merge candidates. */
>  void get_temp_registers_remapping(void *mem_ctx, int ntemps,
> @@ -1361,10 +1379,18 @@ void get_temp_registers_remapping(void
> *mem_ctx, int ntemps,
>     register_merge_record *first_erase = reg_access_end;
>     register_merge_record *search_start = trgt + 1;
> 
> +   int rename_distance;
> +
>     while (trgt != reg_access_end) {
>        register_merge_record *src = find_next_rename(search_start,
> reg_access_end,
>                                              trgt->end);
> -      if (src != reg_access_end) {
> +      //if (src != reg_access_end) {
> +
> +      rename_distance = src->begin - search_start->begin;
> +
> +      if ((src != reg_access_end) &&
> +          (should_merge(rename_distance))) {
> +
>           result[src->reg].new_reg = trgt->reg;
>           result[src->reg].valid = true;
>           trgt->end = src->end;