[Mesa-dev] [PATCH 3/5] i965/fs: Emit shader w/a for Gen6 gather

[Kenneth Graunke]

On 02/03/2014 01:29 AM, Chris Forbes wrote:
> Signed-off-by: Chris Forbes <chrisf at ijw.co.nz>
> ---
>  src/mesa/drivers/dri/i965/brw_fs.h           |  1 +
>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 26 ++++++++++++++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
> index 9c5c13a..3d668b9 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -360,6 +360,7 @@ public:
>                                fs_reg shadow_comp, fs_reg lod, fs_reg lod2,
>                                fs_reg sample_index, fs_reg mcs, int sampler);
>     fs_reg emit_mcs_fetch(ir_texture *ir, fs_reg coordinate, int sampler);
> +   void emit_gen6_gather_wa(uint8_t wa, fs_reg dst);
>     fs_reg fix_math_operand(fs_reg src);
>     fs_inst *emit_math(enum opcode op, fs_reg dst, fs_reg src0);
>     fs_inst *emit_math(enum opcode op, fs_reg dst, fs_reg src0, fs_reg src1);
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> index d88d24c..109f2e8 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> @@ -1699,9 +1699,35 @@ fs_visitor::visit(ir_texture *ir)
>        }
>     }
>  
> +   if (brw->gen == 6 && ir->op == ir_tg4 && c->key.tex.gen6_gather_wa[sampler]) {

I think it might be easier to read if you did:

   if (brw->gen == 6 && ir->op == ir_tg4) {
      emit_gen6_gather_wa(c->key.tex.gen6_gather_wa[sampler], dst);
   }

and then in the body of the function did:

   if (!wa)
      return;

> +      emit_gen6_gather_wa(c->key.tex.gen6_gather_wa[sampler], dst);
> +   }
> +
>     swizzle_result(ir, dst, sampler);
>  }
>  
> +/*

Comments above functions should start with /**, for Doxygen.

> + * Apply workarounds for Gen6 gather with UINT/SINT
> + */
> +void
> +fs_visitor::emit_gen6_gather_wa(uint8_t wa, fs_reg dst)
> +{
> +   int width = (wa & WA_8BIT) ? 8 : 16;
> +
> +   for (int i = 0; i < 4; i++) {
> +      fs_reg dst_f = dst.retype(BRW_REGISTER_TYPE_F);

Adding a comment would help:

      /* Convert from UNORM to UINT. */

> +      emit(MUL(dst_f, dst_f, fs_reg((float)((1 << width) - 1))));

If you like, you could write this using C++ constructor casts
emit(MUL(dst_f, dst_f, fs_reg(float((1 << width) - 1))));
which reduces the parenthesis jumble a bit.  Either way is fine.

> +      emit(MOV(dst, dst_f));
> +
> +      if (wa & WA_SIGN) {

         /* Reinterpret the UINT value as a signed INT value by shifting
          * the sign bit into place, then shifting back preserving sign.
          */

> +         emit(SHL(dst, dst, fs_reg(32 - width)));
> +         emit(ASR(dst, dst, fs_reg(32 - width)));
> +      }
> +
> +      dst.reg_offset++;
> +   }
> +}
> +
>  /**
>   * Set up the gather channel based on the swizzle, for gather4.
>   */
> 

These suggestions apply to patch 4 as well.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140204/0ca82026/attachment-0001.pgp>