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

[Francisco Jerez]

Paul Berry <stereotype441 at gmail.com> writes:

> 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?
>

I haven't, but I'm aware of this problem.  It's one of the things I wish
I had finished before I left.  I had a few solutions in mind, all of
them likely to be time-consuming and not especially compelling:

 - Rename what I called backend_reg to e.g. base_reg, make it an
   abstract base class or use some other technique to prevent its
   construction.  Now backend_reg would be implemented as a sort of
   type-erased wrapper for the other kinds of regs.  This wouldn't be
   too hard because the only thing we need backend_regs for is to pass
   values around, the actual uses of those regs know about the real
   register types.

 - Prevent construction of backend_reg as before, but pass them around
   as pointers instead of by value.  This would be simple but kind of
   annoying and inconsistent with the current value semantics of the
   other register classes.

 - Make backend_reg a member variable rather than a base class to
   prevent unsafe up-casting.  We could have helper functions to convert
   a fs/vec4 reg into a backend reg that check if the original register
   region parameters are the identity mapping and, if they are not, copy
   the value over to some temporary storage.  Assignments would be
   handled by the visitor itself rather than the surface lowering code.

 - Use static rather than dynamic polymorphism in the shared code, which
   would be parameterized by the correct register type.  There is really
   no reason for the shared code to be a (polymorphic) class, other than
   to avoid templates.

I think right now I'm leaning towards possibility 1, but I'll give the
matter another thought and try to get one of these solutions working
when I find the time.  I can't promise when that will happen though.

Let me know if you come up with anything else.

Thanks.

> 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 --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131219/cbec9ff9/attachment.pgp>