On 3 November 2015 at 17:55, Matt Turner <matts...@gmail.com> wrote:
> On Tue, Nov 3, 2015 at 8:04 AM, Emil Velikov <emil.l.veli...@gmail.com> wrote:
>> On 3 November 2015 at 00:29, Matt Turner <matts...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
