[Mesa-dev] [PATCH 08/24] i965: Remove fixed_hw_reg field from backend_reg.
Matt Turner
mattst88 at gmail.com
Tue Nov 3 09:55:14 PST 2015
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.
>> ---
>> 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().
>
>> 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.
>> 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.
>> @@ -1556,12 +1556,15 @@ fs_visitor::assign_vs_urb_setup()
>> inst->src[i].reg +
>> inst->src[i].reg_offset;
>>
>> - inst->src[i].file = HW_REG;
>> - inst->src[i].fixed_hw_reg =
>> + struct brw_reg reg =
>> stride(byte_offset(retype(brw_vec8_grf(grf, 0), inst->src[i].type),
>> inst->src[i].subreg_offset),
>> inst->exec_size * inst->src[i].stride,
>> inst->exec_size, inst->src[i].stride);
>> + reg.abs = inst->src[i].abs;
>> + reg.negate = inst->src[i].negate;
>> +
>> + inst->src[i] = reg;
> Similar concerns as above.
>
>> 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.
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp b/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> index 74d26da..ad52c9f 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> @@ -119,25 +119,24 @@ src_reg::src_reg(uint8_t vf0, uint8_t vf1, uint8_t vf2, uint8_t vf3)
>> (vf3 << 24);
>> }
>>
>> -src_reg::src_reg(struct brw_reg reg)
>> +src_reg::src_reg(struct brw_reg reg) :
>> + backend_reg(reg)
>> {
>> - init();
>> -
> Might as well keep the initI() for now (here and below)...
Explained above.
>> this->file = HW_REG;
>> - this->fixed_hw_reg = reg;
>> - this->type = reg.type;
>
>> + this->reg = 0;
>> + this->reg_offset = 0;
>> + this->swizzle = BRW_SWIZZLE_XXXX;
>> + this->reladdr = NULL;
> ... and drop these (here and below)?
>
>> }
>>
>> -src_reg::src_reg(const dst_reg ®)
>> +src_reg::src_reg(const dst_reg ®) :
>> + backend_reg(static_cast<struct brw_reg>(reg))
>> {
>> - init();
>> -
>> this->file = reg.file;
>> this->reg = reg.reg;
>> this->reg_offset = reg.reg_offset;
>> this->type = reg.type;
> Should we remove this line ?
Seems like it, yes!
>> this->reladdr = reg.reladdr;
>> - this->fixed_hw_reg = reg.fixed_hw_reg;
>> this->swizzle = brw_swizzle_for_mask(reg.writemask);
>> }
>>
>> @@ -184,26 +183,25 @@ dst_reg::dst_reg(register_file file, int reg, brw_reg_type type,
>> this->writemask = writemask;
>> }
>>
>> -dst_reg::dst_reg(struct brw_reg reg)
>> +dst_reg::dst_reg(struct brw_reg reg) :
>> + backend_reg(reg)
>> {
>> - init();
>> -
>> this->file = HW_REG;
>> - this->fixed_hw_reg = reg;
>> - this->type = reg.type;
>> + this->reg = 0;
>> + this->reg_offset = 0;
>> + this->writemask = WRITEMASK_XYZW;
>> + this->reladdr = NULL;
>> }
>>
>> -dst_reg::dst_reg(const src_reg ®)
>> +dst_reg::dst_reg(const src_reg ®) :
>> + backend_reg(static_cast<struct brw_reg>(reg))
>> {
>> - init();
>> -
>> this->file = reg.file;
>> this->reg = reg.reg;
>> this->reg_offset = reg.reg_offset;
>> this->type = reg.type;
> Should we remove this line ?
Yep.
>> this->writemask = brw_mask_for_swizzle(reg.swizzle);
>> this->reladdr = reg.reladdr;
>> - this->fixed_hw_reg = reg.fixed_hw_reg;
>> }
>>
>
>> @@ -1596,8 +1586,7 @@ vec4_visitor::lower_attributes_to_hw_regs(const int *attribute_map,
>> reg.type = inst->dst.type;
>> reg.writemask = inst->dst.writemask;
>>
>> - inst->dst.file = HW_REG;
>> - inst->dst.fixed_hw_reg = reg;
>> + inst->dst = reg;
> Same concern, as in fs_visitor::assign_curb_setup. Past the struct
> brw_reg, dst_reg will be uninitialized. Or is the
> dst_reg::dst_reg(struct brw_reg reg) ctor going to kick in here ?
Same thing as above -- none of those fields contain any meaningful
data with HW_REGs.
>> }
>>
>> for (int i = 0; i < 3; i++) {
>> @@ -1619,8 +1608,7 @@ vec4_visitor::lower_attributes_to_hw_regs(const int *attribute_map,
>> if (inst->src[i].negate)
>> reg = negate(reg);
>>
>> - inst->src[i].file = HW_REG;
>> - inst->src[i].fixed_hw_reg = reg;
>> + inst->src[i] = reg;
> Ditto
>
>
>> @@ -1844,7 +1831,7 @@ vec4_visitor::convert_to_hw_regs()
>> case ATTR:
>> unreachable("not reached");
>> }
>> - src.fixed_hw_reg = reg;
>> + src = reg;
> Ditto (and the rest of vec4_visitor::convert_to_hw_regs() )
>
>
> -Emil
More information about the mesa-dev
mailing list