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

[Kenneth Graunke]

On Saturday, June 27, 2015 05:00:22 PM Jordan Justen wrote:
> 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

*shrug*.  It doesn't really affect me, and I don't have a strong
preference one way or another.  I mostly wanted there to be a sensible
place to handle MESA_SHADER_GEOMETRY.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150627/7375458b/attachment.sig>