[Mesa-dev] [PATCH 06/23] i965: Define common register base class shared between both back-ends.

[Paul Berry]

On 2 December 2013 11:31, Francisco Jerez <currojerez at riseup.net> wrote:

> diff --git a/src/mesa/drivers/dri/i965/brw_shader.h
> b/src/mesa/drivers/dri/i965/brw_shader.h
> index ff5af93..f284389 100644
> --- a/src/mesa/drivers/dri/i965/brw_shader.h
> +++ b/src/mesa/drivers/dri/i965/brw_shader.h
> @@ -23,6 +23,7 @@
>
>  #include <stdint.h>
>  #include "brw_defines.h"
> +#include "brw_reg.h"
>  #include "glsl/ir.h"
>
>  #pragma once
> @@ -39,6 +40,45 @@ enum register_file {
>
>  #ifdef __cplusplus
>
> +class backend_reg {
> +public:
> +   backend_reg();
> +   backend_reg(struct brw_reg reg);
> +
> +   bool is_zero() const;
> +   bool is_one() const;
> +   bool is_null() const;
> +
> +   /** Register file: GRF, MRF, IMM. */
> +   enum register_file file;
> +
> +   /**
> +    * Register number.  For MRF, it's the hardware register.  For
> +    * GRF, it's a virtual register number until register allocation
> +    */
> +   int reg;
> +
> +   /**
> +    * Offset from the start of the contiguous register block.
> +    *
> +    * For pre-register-allocation GRFs, this is in units of a float per
> pixel
> +    * (1 hardware register for SIMD8 mode, or 2 registers for SIMD16
> mode).
>

Since this code is now shared with the vec4 back-end, can we update this
comment to say "1 hardware register for SIMD8 or SIMD4x2 mode..."?


> +    * For uniforms, this is in units of 1 float.
> +    */
> +   int reg_offset;
> +
> +   /** Register type.  BRW_REGISTER_TYPE_* */
> +   int type;
> +   struct brw_reg fixed_hw_reg;
> +
> +   /** Value for file == BRW_IMMMEDIATE_FILE */
> +   union {
> +      int32_t i;
> +      uint32_t u;
> +      float f;
> +   } imm;
> +};
> +
>  class backend_instruction : public exec_node {
>  public:
>     bool is_tex();
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h
> b/src/mesa/drivers/dri/i965/brw_vec4.h
> index 3b3f35b..a718333 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4.h
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
> @@ -89,28 +89,7 @@ class dst_reg;
>  unsigned
>  swizzle_for_size(int size);
>
> -class reg
> -{
> -public:
> -   /** Register file: GRF, MRF, IMM. */
> -   enum register_file file;
> -   /** virtual register number.  0 = fixed hw reg */
> -   int reg;
> -   /** Offset within the virtual register. */
> -   int reg_offset;
> -   /** Register type.  BRW_REGISTER_TYPE_* */
> -   int type;
> -   struct brw_reg fixed_hw_reg;
> -
> -   /** Value for file == BRW_IMMMEDIATE_FILE */
> -   union {
> -      int32_t i;
> -      uint32_t u;
> -      float f;
> -   } imm;
> -};
> -
> -class src_reg : public reg
> +class src_reg : public backend_reg
>  {
>  public:
>     DECLARE_RALLOC_CXX_OPERATORS(src_reg)
> @@ -123,10 +102,9 @@ public:
>     src_reg(uint32_t u);
>     src_reg(int32_t i);
>     src_reg(struct brw_reg reg);
> +   src_reg(const backend_reg &reg);
>

I'm concerned about this constructor (and the corresponding constructors in
the dst_reg and fs_reg classes) contributing to object slicing problems.
Consider this code:

    src_reg r1;
    r1.swizzle = BRW_SWIZZLE_XXXX;
    backend_reg r2(r1);
    src_reg r3(r2);
    assert(r3.swizzle == BRW_SWIZZLE_XXXX); /* fails! */

The reason for the failure is that src_reg::swizzle doesn't appear in the
backend_reg base class.  So when r1 is copied to r2, the value of swizzle
is lost.  That by itself wouldn't be a problem, except that when we later
try to reconstruct a src_reg from r2, swizzle winds up being initialized to
the default value.

Of course we would never write code like the above example directly, but
since your shared code for implementing loads/stores (in future patches)
uses backend_reg for all of its register representations, it seems likely
that a src_reg will get converted to a backend_reg and then back to a
src_reg at some point during compilation.  So I suspect that when compiling
GLSL source code like the following:

    uniform image2D img;
    ivec2 coord;
    vec4 v;
    imageStore(img, coord, v.yzwx);

The swizzle would fail to take effect.

Have you run tests to see if this is in fact the case?

Unfortunately I don't really have a good suggestion as to what to do about
this.  I'll continue reviewing the series assuming this problem isn't
present, but I think we need to figure out a solution before we can land
the series.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131219/819eb7cc/attachment.html>