[Mesa-dev] [PATCH 08/24] i965: Remove fixed_hw_reg field from backend_reg.

Tue Nov 3 10:45:43 PST 2015

On 3 November 2015 at 17:55, Matt Turner <mattst88 at gmail.com> wrote:
> On Tue, Nov 3, 2015 at 8:04 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 3 November 2015 at 00:29, Matt Turner <mattst88 at gmail.com> wrote:
>>
>> Please add a bit of commit message - "Mostly unused as of last commit.
>> Fold the remaining cases (GRF only?) to use the base brw_reg struct."
>> or anything else that you feel is appropriate.
>
> How about
>
>    Since backend_reg now inherits brw_reg, we can use it in place of the
>    fixed_hw_reg field.
>
Quite better. Thanks.

>>> ---
>>>  src/mesa/drivers/dri/i965/brw_fs.cpp               |  93 +++++++++---------
>>>  src/mesa/drivers/dri/i965/brw_fs_generator.cpp     |   9 +-
>>>  src/mesa/drivers/dri/i965/brw_fs_reg_allocate.cpp  |   4 +-
>>>  src/mesa/drivers/dri/i965/brw_ir_fs.h              |   4 +-
>>>  src/mesa/drivers/dri/i965/brw_ir_vec4.h            |   4 +-
>>>  .../drivers/dri/i965/brw_schedule_instructions.cpp |  54 +++++------
>>>  src/mesa/drivers/dri/i965/brw_shader.cpp           |   8 +-
>>>  src/mesa/drivers/dri/i965/brw_shader.h             |   5 +-
>>>  src/mesa/drivers/dri/i965/brw_vec4.cpp             | 108 +++++++++------------
>>>  src/mesa/drivers/dri/i965/brw_vec4_generator.cpp   |  12 +--
>>>  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp     |   2 -
>>>  11 files changed, 141 insertions(+), 162 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> index 6b9e979..243a4ac 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> @@ -422,13 +422,15 @@ fs_reg::fs_reg(uint8_t vf0, uint8_t vf1, uint8_t vf2, uint8_t vf3)
>>>                                 (vf3 << 24);
>>>  }
>>>
>>> -/** Fixed brw_reg. */
>>> -fs_reg::fs_reg(struct brw_reg fixed_hw_reg)
>>> +fs_reg::fs_reg(struct brw_reg reg) :
>>> +   backend_reg(reg)
>>>  {
>>> -   init();
>> Keep this for now ?
>
> I can't -- since this function now uses an initializer list, init()
> would happen after the backend_reg() constructor has run, thereby
> memsetting the object to zero.
>
>>
>>>     this->file = HW_REG;
>>> -   this->fixed_hw_reg = fixed_hw_reg;
>>> -   this->type = fixed_hw_reg.type;
>>
>>> +   this->reg = 0;
>>> +   this->reg_offset = 0;
>>> +   this->subreg_offset = 0;
>>> +   this->reladdr = NULL;
>>> +   this->stride = 1;
>> .. and drop these ?
>
> Can't, without readding init().
>
Yes scratch these comments. Brain froze between "how will things look
if we've initialize things in backend_reg(?) and drop the init()" and
this series.

>>
>>>  fs_reg::fs_reg(enum register_file file, int reg, enum brw_reg_type type)
>>>  {
>>>     init();
>>> @@ -1400,10 +1399,11 @@ fs_visitor::assign_curb_setup()
>>>             struct brw_reg brw_reg = brw_vec1_grf(payload.num_regs +
>>>                                                   constant_nr / 8,
>>>                                                   constant_nr % 8);
>>> +            brw_reg.abs = inst->src[i].abs;
>>> +            brw_reg.negate = inst->src[i].negate;
>>>
>> This looks like a bugfix which might contribute to an occasional
>> random behaviour ?
>
> Actually, this is necessary (and now that I think about it might
> indicate an intermediate breakage earlier in the series).
>
> The problem is that we're reconstructing the fs_reg using the
> fs_reg(brw_reg) constructor (see "inst->src[i] = byte_offset(" below).
> The default abs/negate values for brw_reg are false, so we have to
> copy them over from our fs_reg.
>
> I'll check whether this is fixing a problem introduced by removing the
> abs/negate fields from backend_reg. If it is, some of this will need
> to be moved to that commit.
>
Glad I could help :)

>>>              assert(inst->src[i].stride == 0);
>>> -           inst->src[i].file = HW_REG;
>>> -           inst->src[i].fixed_hw_reg = byte_offset(
>>> +            inst->src[i] = byte_offset(
>> Something looks odd here. We will likely create a brw_reg and the
>> remaining bits of inst->src[i] will be uninitialised as the old object
>> will be torn down. Although I could be missing something.
>
> It shouldn't matter matter. Once we have a brw_reg (with file ==
> HW_REG), none of those fields are meaningful.
>
Indeed.

>>> diff --git a/src/mesa/drivers/dri/i965/brw_shader.h b/src/mesa/drivers/dri/i965/brw_shader.h
>>> index 9a7a2d5..4d9a946 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_shader.h
>>> +++ b/src/mesa/drivers/dri/i965/brw_shader.h
>>> @@ -51,6 +51,9 @@ enum PACKED register_file {
>>>  #ifdef __cplusplus
>>>  struct backend_reg : public brw_reg
>>>  {
>>> +   backend_reg() {}
>>> +   backend_reg(struct brw_reg reg) : brw_reg(reg) {}
>>> +
>> As brw_reg is a normal struct, wouldn't it be better if we initialize
>> it and backend_reg's members in the above two ? Ideally this could be
>> a separate commit (7.1) and patch 7.2 would in turn drop the init()
>> and use the above ctors. If you want to keep it as is, I won't push
>> it.
>
> The thing is, most of the fs_reg/src_reg/dst_reg constructors are
> still memsetting their whole objects to zero, and I don't think the
> compiler will be able to recognize the double initialization.
>
> I think removing the init() function all together (and initializing
> reg_offset here, etc) is worth exploring though.
>
I already have a few patches for what. I've even started exploring
which constructors we can nuke :-)
Can you post a branch somewhere so that I can rebase + send before you
get onto the next batch.

Thanks
Emil