[Mesa-dev] [PATCH] i965: Switch on shader stage in nir_setup_outputs().

[Jordan Justen]

On 2015-06-26 16:03:21, Kenneth Graunke wrote:
> Adding new shader stages to a switch statement is less confusing than an
> if-else-if ladder where all but the first case are fragment shader
> specific (but don't claim to be).
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp |   59 +++++++++++++++++-------------
>  1 file changed, 33 insertions(+), 26 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index 59081ea..8bcd5e2 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -133,38 +133,45 @@ fs_visitor::nir_setup_outputs(nir_shader *shader)
>           var->type->is_array() ? var->type->fields.array->vector_elements
>                                 : var->type->vector_elements;
>  
> -      if (stage == MESA_SHADER_VERTEX) {
> +      switch (stage) {
> +      case MESA_SHADER_VERTEX:
>           for (int i = 0; i < ALIGN(type_size(var->type), 4) / 4; i++) {
>              int output = var->data.location + i;
>              this->outputs[output] = offset(reg, 4 * i);
>              this->output_components[output] = vector_elements;
>           }
> -      } else if (var->data.index > 0) {
> -         assert(var->data.location == FRAG_RESULT_DATA0);
> -         assert(var->data.index == 1);
> -         this->dual_src_output = reg;
> -         this->do_dual_src = true;
> -      } else if (var->data.location == FRAG_RESULT_COLOR) {
> -         /* Writing gl_FragColor outputs to all color regions. */
> -         for (unsigned int i = 0; i < MAX2(key->nr_color_regions, 1); i++) {
> -            this->outputs[i] = reg;
> -            this->output_components[i] = 4;
> -         }
> -      } else if (var->data.location == FRAG_RESULT_DEPTH) {
> -         this->frag_depth = reg;
> -      } else if (var->data.location == FRAG_RESULT_SAMPLE_MASK) {
> -         this->sample_mask = reg;
> -      } else {
> -         /* gl_FragData or a user-defined FS output */
> -         assert(var->data.location >= FRAG_RESULT_DATA0 &&
> -                var->data.location < FRAG_RESULT_DATA0 + BRW_MAX_DRAW_BUFFERS);
> -
> -         /* General color output. */
> -         for (unsigned int i = 0; i < MAX2(1, var->type->length); i++) {
> -            int output = var->data.location - FRAG_RESULT_DATA0 + i;
> -            this->outputs[output] = offset(reg, vector_elements * i);
> -            this->output_components[output] = vector_elements;
> +         break;
> +      case MESA_SHADER_FRAGMENT:
> +         if (var->data.index > 0) {
> +            assert(var->data.location == FRAG_RESULT_DATA0);
> +            assert(var->data.index == 1);
> +            this->dual_src_output = reg;
> +            this->do_dual_src = true;
> +         } else if (var->data.location == FRAG_RESULT_COLOR) {
> +            /* Writing gl_FragColor outputs to all color regions. */
> +            for (unsigned int i = 0; i < MAX2(key->nr_color_regions, 1); i++) {
> +               this->outputs[i] = reg;
> +               this->output_components[i] = 4;
> +            }
> +         } else if (var->data.location == FRAG_RESULT_DEPTH) {
> +            this->frag_depth = reg;
> +         } else if (var->data.location == FRAG_RESULT_SAMPLE_MASK) {
> +            this->sample_mask = reg;
> +         } else {
> +            /* gl_FragData or a user-defined FS output */
> +            assert(var->data.location >= FRAG_RESULT_DATA0 &&
> +                   var->data.location < FRAG_RESULT_DATA0+BRW_MAX_DRAW_BUFFERS);
> +
> +            /* General color output. */
> +            for (unsigned int i = 0; i < MAX2(1, var->type->length); i++) {
> +               int output = var->data.location - FRAG_RESULT_DATA0 + i;
> +               this->outputs[output] = offset(reg, vector_elements * i);
> +               this->output_components[output] = vector_elements;
> +            }

I noticed that it looks like MESA_SHADER_FRAGMENT case could use a
switch as well on var->data.location. Does it help, or is it better to
keep the if-ladder there?

switch (var->data.location) {
   case FRAG_RESULT_COLOR;
      /* do stuff */
      break;
   case FRAG_RESULT_DEPTH:
      /* do stuff */
      break;
   case FRAG_RESULT_SAMPLE_MASK:
      /* do stuff */
      break;
   case FRAG_RESULT_DATA0:
      if (var->data.index > 0) {
         /* do stuff */
         break;
      }
      /* fall-through */
   default:
      assert(var->data.index == 0);
      /* do stuff */
      break;
}

-Jordan

>           }
> +         break;
> +      default:
> +         unreachable("unhandled shader stage");
>        }
>     }
>  }
> -- 
> 1.7.10.4
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev