[Mesa-dev] [PATCH 06/10] i965/fs: Add support for user-defined out variables.

[Kenneth Graunke]

On 11/04/2011 03:01 PM, Eric Anholt wrote:
> Before, I was tracking the ir_variable * found for gl_FragColor or
> gl_FragData[].  Instead, when visiting those variables, set up an
> array of per-render-target fs_regs to copy the output data from.  This
> cleans up the color emit path, while making handling of multiple
> user-defined out variables easier.
> 
> Note that brw_type_for_base_type() now returns a sensible value for
> arrays.  This probably obsoletes a bunch of type overrides we have
> laying around.
> ---
>  src/mesa/drivers/dri/i965/brw_fs.h           |    8 +-
>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp |   80 ++++++++++++++-----------
>  src/mesa/drivers/dri/i965/brw_shader.cpp     |    1 +
>  3 files changed, 50 insertions(+), 39 deletions(-)

I like these changes.  However, it feels like you're really doing three
separate changes in one patch.  I went ahead and split it up, and will
post those shortly.

A few more comments inline.

> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
> index e2ad649..854a935 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
[snip]
> @@ -77,9 +69,32 @@ fs_visitor::visit(ir_variable *ir)
>        assert(reg);
>        hash_table_insert(this->variable_ht, reg, ir);
>        return;
> -   }
> +   } else if (ir->mode == ir_var_out) {
> +      reg = new(this->mem_ctx) fs_reg(this, ir->type);
> +
> +      if (strcmp(ir->name, "gl_FragColor") == 0) {
> +	 for (int i = 0; i < c->key.nr_color_regions; i++) {
> +	    this->outputs[i] = *reg;
> +	 }
> +      } else if (strcmp(ir->name, "gl_FragData") == 0) {
> +	 for (int i = 0; i < c->key.nr_color_regions; i++) {
> +	    this->outputs[i] = *reg;
> +	    this->outputs[i].reg_offset += 4 * i;
> +	 }
> +      } else if (strcmp(ir->name, "gl_FragDepth") == 0) {
> +	 this->frag_depth = ir;
> +      } else {
> +	 assert(ir->location >= FRAG_RESULT_DATA0 &&
> +		ir->location < FRAG_RESULT_DATA0 + BRW_MAX_DRAW_BUFFERS);
>  
> -   if (ir->mode == ir_var_uniform) {
> +	 /* General color output. */
> +	 for (unsigned int i = 0; i < MAX2(1, ir->type->length); i++) {
> +	    int output = ir->location - FRAG_RESULT_DATA0 + i;
> +	    this->outputs[output] = *reg;
> +	    this->outputs[output].reg_offset += 4 * i;
> +	 }
> +      }
> +   } else if (ir->mode == ir_var_uniform) {
>        int param_index = c->prog_data.nr_params;
>  
>        if (c->dispatch_width == 16) {

I agree with Ian here---using ir->location ought to be cleaner.  The
code for gl_FragData and generic FS outputs is exactly the same...

I've gone ahead and made this change too, I'll post that shortly.

[snip]
> @@ -1988,13 +1999,12 @@ fs_visitor::emit_fb_writes()
>     }
>  
>     if (c->key.nr_color_regions == 0) {
> -      if (c->key.alpha_test && (this->frag_color || this->frag_data)) {
> +      if (c->key.alpha_test) {
>  	 /* If the alpha test is enabled but there's no color buffer,
>  	  * we still need to send alpha out the pipeline to our null
>  	  * renderbuffer.
>  	  */
> -	 color.reg_offset += 3;
> -	 emit_color_write(3, color_mrf, color);
> +	 emit_color_write(0, 3, color_mrf);
>        }

I'm a little concerned about removing reg_offset += 3.  This seems like
a change in behavior (assigning all four instead of just alpha).  Was it
wrong before/is this a bug fix?  Or, is this just simpler code (since
the other three channels are ignored anyway)?

--Kenneth