[Mesa-dev] [PATCH 08/24] i965: Remove fixed_hw_reg field from backend_reg.
Emil Velikov
emil.l.velikov at gmail.com
Tue Nov 3 08:04:46 PST 2015
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.
> ---
> 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 ?
> 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 ?
> 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 ?
> 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.
> @@ -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.
> 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)...
> 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 ?
> 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 ?
> 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 ?
> }
>
> 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