On Fri, Feb 21, 2014 at 03:23:42PM -0800, Kenneth Graunke wrote:
> Using this emit function implicitly creates three copies, which
> is pointlessly inefficient.
>
> 1. Code creates the original instruction.
> 2. Calling emit(fs_inst) copies it into the function.
> 3. It then allocates a new fs_inst and copies it into that.
>
> The second could be eliminated by changing the signature to
>
> fs_inst(const fs_inst &)
>
> but that wouldn't eliminate the third. Making callers heap allocate the
> instruction and call emit(fs_inst *) allows us to just use the original
> one, with no extra copies, and isn't much more of a burden.
Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
>
> Cc: Matt Turner <matts...@gmail.com>
> Signed-off-by: Kenneth Graunke <kenn...@whitecape.org>
> ---
> src/mesa/drivers/dri/i965/brw_fs.cpp | 14 +++++++-------
> src/mesa/drivers/dri/i965/brw_fs.h | 1 -
> src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 27 ++++++++++-----------------
> 3 files changed, 17 insertions(+), 25 deletions(-)
>
> FAIL. This is the patch I actually meant to send. :)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
> b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 65f2c80..5af3397 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -641,8 +641,8 @@ fs_visitor::emit_shader_time_write(enum
> shader_time_shader_type type,
> else
> payload = fs_reg(this, glsl_type::uint_type);
>
> - emit(fs_inst(SHADER_OPCODE_SHADER_TIME_ADD,
> - fs_reg(), payload, offset, value));
> + emit(new(mem_ctx) fs_inst(SHADER_OPCODE_SHADER_TIME_ADD,
> + fs_reg(), payload, offset, value));
> }
>
> void
> @@ -671,32 +671,32 @@ fs_visitor::fail(const char *format, ...)
> fs_inst *
> fs_visitor::emit(enum opcode opcode)
> {
> - return emit(fs_inst(opcode));
> + return emit(new(mem_ctx) fs_inst(opcode));
> }
>
> fs_inst *
> fs_visitor::emit(enum opcode opcode, fs_reg dst)
> {
> - return emit(fs_inst(opcode, dst));
> + return emit(new(mem_ctx) fs_inst(opcode, dst));
> }
>
> fs_inst *
> fs_visitor::emit(enum opcode opcode, fs_reg dst, fs_reg src0)
> {
> - return emit(fs_inst(opcode, dst, src0));
> + return emit(new(mem_ctx) fs_inst(opcode, dst, src0));
> }
>
> fs_inst *
> fs_visitor::emit(enum opcode opcode, fs_reg dst, fs_reg src0, fs_reg src1)
> {
> - return emit(fs_inst(opcode, dst, src0, src1));
> + return emit(new(mem_ctx) fs_inst(opcode, dst, src0, src1));
> }
>
> fs_inst *
> fs_visitor::emit(enum opcode opcode, fs_reg dst,
> fs_reg src0, fs_reg src1, fs_reg src2)
> {
> - return emit(fs_inst(opcode, dst, src0, src1, src2));
> + return emit(new(mem_ctx) fs_inst(opcode, dst, src0, src1, src2));
> }
>
> void
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h
> b/src/mesa/drivers/dri/i965/brw_fs.h
> index b1e38b6..ebf90a3 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -282,7 +282,6 @@ public:
>
> bool can_do_source_mods(fs_inst *inst);
>
> - fs_inst *emit(fs_inst inst);
> fs_inst *emit(fs_inst *inst);
> void emit(exec_list list);
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> index aea3360..b1e795e 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> @@ -741,8 +741,8 @@ fs_visitor::visit(ir_expression *ir)
> packed_consts.type = result.type;
>
> fs_reg const_offset_reg = fs_reg(const_offset->value.u[0] & ~15);
> - emit(fs_inst(FS_OPCODE_UNIFORM_PULL_CONSTANT_LOAD,
> - packed_consts, surf_index, const_offset_reg));
> + emit(new(mem_ctx) fs_inst(FS_OPCODE_UNIFORM_PULL_CONSTANT_LOAD,
> + packed_consts, surf_index,
> const_offset_reg));
>
> for (int i = 0; i < ir->type->vector_elements; i++) {
> packed_consts.set_smear(const_offset->value.u[0] % 16 / 4 + i);
> @@ -2397,9 +2397,10 @@ fs_visitor::emit_untyped_atomic(unsigned atomic_op,
> unsigned surf_index,
> }
>
> /* Emit the instruction. */
> - fs_inst inst(SHADER_OPCODE_UNTYPED_ATOMIC, dst, atomic_op, surf_index);
> - inst.base_mrf = 0;
> - inst.mlen = mlen;
> + fs_inst *inst = new(mem_ctx) fs_inst(SHADER_OPCODE_UNTYPED_ATOMIC, dst,
> + atomic_op, surf_index);
> + inst->base_mrf = 0;
> + inst->mlen = mlen;
> emit(inst);
> }
>
> @@ -2430,22 +2431,14 @@ fs_visitor::emit_untyped_surface_read(unsigned
> surf_index, fs_reg dst,
> mlen += operand_len;
>
> /* Emit the instruction. */
> - fs_inst inst(SHADER_OPCODE_UNTYPED_SURFACE_READ, dst, surf_index);
> - inst.base_mrf = 0;
> - inst.mlen = mlen;
> + fs_inst *inst = new(mem_ctx)
> + fs_inst(SHADER_OPCODE_UNTYPED_SURFACE_READ, dst, surf_index);
> + inst->base_mrf = 0;
> + inst->mlen = mlen;
> emit(inst);
> }
>
> fs_inst *
> -fs_visitor::emit(fs_inst inst)
> -{
> - fs_inst *list_inst = new(mem_ctx) fs_inst;
> - *list_inst = inst;
> - emit(list_inst);
> - return list_inst;
> -}
> -
> -fs_inst *
> fs_visitor::emit(fs_inst *inst)
> {
> if (force_uncompressed_stack > 0)
> --
> 1.9.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
