[Mesa-dev] [PATCH 2/2] i965: Prevent implicit upcasts to brw_reg.
Francisco Jerez
currojerez at riseup.net
Fri Nov 20 05:25:01 PST 2015
Matt Turner <mattst88 at gmail.com> writes:
> Now that backend_reg inherits from brw_reg, we have to be careful to
> avoid the object slicing problem.
> ---
> src/mesa/drivers/dri/i965/brw_fs.cpp | 6 +++--
> .../drivers/dri/i965/brw_fs_copy_propagation.cpp | 4 +--
> src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 2 +-
> src/mesa/drivers/dri/i965/brw_shader.h | 31 +++++++++++++++++++++-
> src/mesa/drivers/dri/i965/brw_vec4.cpp | 15 ++++++-----
> .../drivers/dri/i965/brw_vec4_copy_propagation.cpp | 4 +--
> src/mesa/drivers/dri/i965/brw_vec4_generator.cpp | 10 +++----
> 7 files changed, 53 insertions(+), 19 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index e9c990d..a5f1d8f 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -392,7 +392,8 @@ fs_reg::fs_reg(struct brw_reg reg) :
> bool
> fs_reg::equals(const fs_reg &r) const
> {
> - return (memcmp((brw_reg *)this, (brw_reg *)&r, sizeof(brw_reg)) == 0 &&
> + return (memcmp(&this->as_brw_reg(), &r.as_brw_reg(),
> + sizeof(struct brw_reg)) == 0 &&
> reg_offset == r.reg_offset &&
> subreg_offset == r.subreg_offset &&
> !reladdr && !r.reladdr &&
> @@ -2037,7 +2038,8 @@ fs_visitor::opt_algebraic()
> if (inst->dst.type != inst->src[0].type)
> assert(!"unimplemented: saturate mixed types");
>
> - if (brw_saturate_immediate(inst->dst.type, &inst->src[0])) {
> + if (brw_saturate_immediate(inst->dst.type,
> + &inst->src[0].as_brw_reg())) {
> inst->saturate = false;
> progress = true;
> }
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> index 426ea57..b72fe33 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> @@ -478,14 +478,14 @@ fs_visitor::try_constant_propagate(fs_inst *inst, acp_entry *entry)
>
> if (inst->src[i].abs) {
> if ((devinfo->gen >= 8 && is_logic_op(inst->opcode)) ||
> - !brw_abs_immediate(val.type, &val)) {
> + !brw_abs_immediate(val.type, &val.as_brw_reg())) {
> continue;
> }
> }
>
> if (inst->src[i].negate) {
> if ((devinfo->gen >= 8 && is_logic_op(inst->opcode)) ||
> - !brw_negate_immediate(val.type, &val)) {
> + !brw_negate_immediate(val.type, &val.as_brw_reg())) {
> continue;
> }
> }
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> index 77969c4..ea37ea4 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> @@ -92,7 +92,7 @@ brw_reg_from_fs_reg(fs_inst *inst, fs_reg *reg, unsigned gen)
> case ARF:
> case FIXED_GRF:
> case IMM:
> - brw_reg = *static_cast<struct brw_reg *>(reg);
> + brw_reg = reg->as_brw_reg();
> break;
> case BAD_FILE:
> /* Probably unused. */
> diff --git a/src/mesa/drivers/dri/i965/brw_shader.h b/src/mesa/drivers/dri/i965/brw_shader.h
> index a4139cf..52b9aff 100644
> --- a/src/mesa/drivers/dri/i965/brw_shader.h
> +++ b/src/mesa/drivers/dri/i965/brw_shader.h
> @@ -39,11 +39,21 @@
> #define MAX_VGRF_SIZE 16
>
> #ifdef __cplusplus
> -struct backend_reg : public brw_reg
> +struct backend_reg : private brw_reg
> {
> backend_reg() {}
> backend_reg(struct brw_reg reg) : brw_reg(reg) {}
>
> + const brw_reg &as_brw_reg() const
> + {
It would be nice to have assertions here to make sure that the
additional state of the backend reg is the identity, so we know we
aren't losing information by slicing. If you don't feel like overriding
this function in each one of the backend_reg subclasses (which would be
necessary to make sure that e.g. fs_reg::subreg_offset and ::reladdr are
zero and ::stride is one), I guess that for the moment we could just
assert that reg_offset is zero and that file is one of the actual
hardware values (ARF/FIXED_GRF/MRF/IMM).
Note that that will mean that you are no longer able to use these
casting functions to implement some constructors and the equals()
methods of the backend_reg subclasses, so you may have to factor out the
common logic of equals() (i.e. the memcmp call and reg_offset
comparison) into a backend_reg::equals() method (which seems like a good
idea on its own). As for the constructors, is there any reason you
cannot delegate to the implicitly-declared copy constructor of
backend_reg? (That way you'd also avoid initializing reg_offset
manually)
> + return static_cast<const brw_reg &>(*this);
> + }
> +
> + brw_reg &as_brw_reg()
> + {
> + return static_cast<brw_reg &>(*this);
> + }
> +
> bool is_zero() const;
> bool is_one() const;
> bool is_negative_one() const;
> @@ -61,6 +71,25 @@ struct backend_reg : public brw_reg
> * For uniforms, this is in units of 1 float.
> */
> uint16_t reg_offset;
> +
> + using brw_reg::type;
> + using brw_reg::file;
> + using brw_reg::negate;
> + using brw_reg::abs;
> + using brw_reg::address_mode;
> + using brw_reg::subnr;
> + using brw_reg::nr;
> +
> + using brw_reg::swizzle;
> + using brw_reg::writemask;
> + using brw_reg::indirect_offset;
> + using brw_reg::vstride;
> + using brw_reg::width;
> + using brw_reg::hstride;
> +
> + using brw_reg::f;
> + using brw_reg::d;
> + using brw_reg::ud;
> };
> #endif
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> index 44893e3..f564ce1 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> @@ -79,7 +79,7 @@ src_reg::src_reg(struct brw_reg reg) :
> }
>
> src_reg::src_reg(const dst_reg ®) :
> - backend_reg(static_cast<struct brw_reg>(reg))
> + backend_reg(reg.as_brw_reg())
> {
> this->reg_offset = reg.reg_offset;
> this->reladdr = reg.reladdr;
> @@ -137,7 +137,7 @@ dst_reg::dst_reg(struct brw_reg reg) :
> }
>
> dst_reg::dst_reg(const src_reg ®) :
> - backend_reg(static_cast<struct brw_reg>(reg))
> + backend_reg(reg.as_brw_reg())
> {
> this->reg_offset = reg.reg_offset;
> this->writemask = brw_mask_for_swizzle(reg.swizzle);
> @@ -147,7 +147,8 @@ dst_reg::dst_reg(const src_reg ®) :
> bool
> dst_reg::equals(const dst_reg &r) const
> {
> - return (memcmp((brw_reg *)this, (brw_reg *)&r, sizeof(brw_reg)) == 0 &&
> + return (memcmp(&this->as_brw_reg(), &r.as_brw_reg(),
> + sizeof(struct brw_reg)) == 0 &&
> reg_offset == r.reg_offset &&
> (reladdr == r.reladdr ||
> (reladdr && r.reladdr && reladdr->equals(*r.reladdr))));
> @@ -285,7 +286,8 @@ vec4_visitor::implied_mrf_writes(vec4_instruction *inst)
> bool
> src_reg::equals(const src_reg &r) const
> {
> - return (memcmp((brw_reg *)this, (brw_reg *)&r, sizeof(brw_reg)) == 0 &&
> + return (memcmp(&this->as_brw_reg(), &r.as_brw_reg(),
> + sizeof(struct brw_reg)) == 0 &&
> reg_offset == r.reg_offset &&
> !reladdr && !r.reladdr);
> }
> @@ -587,7 +589,8 @@ vec4_visitor::opt_algebraic()
> if (inst->dst.type != inst->src[0].type)
> assert(!"unimplemented: saturate mixed types");
>
> - if (brw_saturate_immediate(inst->dst.type, &inst->src[0])) {
> + if (brw_saturate_immediate(inst->dst.type,
> + &inst->src[0].as_brw_reg())) {
> inst->saturate = false;
> progress = true;
> }
> @@ -1769,7 +1772,7 @@ vec4_visitor::convert_to_hw_regs()
>
> case ARF:
> case FIXED_GRF:
> - reg = dst;
> + reg = dst.as_brw_reg();
> break;
>
> case BAD_FILE:
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> index 3b76e36..ce5f7ab 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> @@ -134,14 +134,14 @@ try_constant_propagate(const struct brw_device_info *devinfo,
>
> if (inst->src[arg].abs) {
> if ((devinfo->gen >= 8 && is_logic_op(inst->opcode)) ||
> - !brw_abs_immediate(value.type, &value)) {
> + !brw_abs_immediate(value.type, &value.as_brw_reg())) {
> return false;
> }
> }
>
> if (inst->src[arg].negate) {
> if ((devinfo->gen >= 8 && is_logic_op(inst->opcode)) ||
> - !brw_negate_immediate(value.type, &value)) {
> + !brw_negate_immediate(value.type, &value.as_brw_reg())) {
> return false;
> }
> }
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> index 20107ac..c06fb21 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> @@ -356,7 +356,7 @@ generate_gs_urb_write_allocate(struct brw_codegen *p, vec4_instruction *inst)
>
> /* We pass the temporary passed in src0 as the writeback register */
> brw_urb_WRITE(p,
> - inst->src[0], /* dest */
> + inst->src[0].as_brw_reg(), /* dest */
> inst->base_mrf, /* starting mrf reg nr */
> src,
> BRW_URB_WRITE_ALLOCATE_COMPLETE,
> @@ -369,8 +369,8 @@ generate_gs_urb_write_allocate(struct brw_codegen *p, vec4_instruction *inst)
> brw_push_insn_state(p);
> brw_set_default_access_mode(p, BRW_ALIGN_1);
> brw_set_default_mask_control(p, BRW_MASK_DISABLE);
> - brw_MOV(p, get_element_ud(inst->dst, 0),
> - get_element_ud(inst->src[0], 0));
> + brw_MOV(p, get_element_ud(inst->dst.as_brw_reg(), 0),
> + get_element_ud(inst->src[0].as_brw_reg(), 0));
> brw_pop_insn_state(p);
> }
>
> @@ -1059,9 +1059,9 @@ generate_code(struct brw_codegen *p,
> annotate(p->devinfo, &annotation, cfg, inst, p->next_insn_offset);
>
> for (unsigned int i = 0; i < 3; i++) {
> - src[i] = inst->src[i];
> + src[i] = inst->src[i].as_brw_reg();
> }
> - dst = inst->dst;
> + dst = inst->dst.as_brw_reg();
>
> brw_set_default_predicate_control(p, inst->predicate);
> brw_set_default_predicate_inverse(p, inst->predicate_inverse);
> --
> 2.4.9
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151120/8ff0da55/attachment.sig>
More information about the mesa-dev
mailing list