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 <kenn...@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.
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
