Kenneth Graunke <kenn...@whitecape.org> writes: > We did this for fs_reg a while back, and it's generally a good idea. > I disagree, explicit constructors aren't a one-size-fits-all. IMO there are three scenarios in which explicit constructors may be a good idea:
- Cases where your constructor may lose relevant information about its
argument when used inadvertently. IOW there is a many-to-one mapping
between your argument type and your constructed type.
- Cases where your constructor doesn't leave the constructed object
completely initialized and some additional action may be required to
bring the constructed object to a well-defined state. IOW there is a
one-to-many mapping between your argument type and your constructed
type.
- Cases where your constructor has to do some expensive or run-time
environment-dependent operation.
If none of these apply your argument and constructed objects are
effectively the same thing, and declaring the constructor explicit just
adds clutter and increases the amount of typing you have to do for no
benefit. I suspect that the immediate register constructors from both
back-ends don't fit in any of the three categories, they do the only
sane thing they could possibly do without losing any information, so I
don't see why we would want them to be explicit.
Actually it would make it rather annoying to pass immediates around with
the i965 IR builder framework I'm working on for
ARB_shader_image_load_store unless I change my src_vector type to have a
constructor for each immediate type instead of relying on the implicit
conversion to src/fs_reg, but then I'd have to maintain another
constructor for each possible src/fs_reg constructor argument and keep
them up to date.
I agree though that there is a good reason for the src_reg(dst_reg)
constructor and its converse to be marked explicit, because they
(currently) lose information. dst_reg(src_reg) necessarily loses
component ordering information because you cannot represent that as a
writemask, the transformation could be better behaved than what we have
if it calculated the subset of components referenced by the swizzle of
its argument instead of special-casing XXXX.
There's no good reason why src_reg(dst_reg) should lose information, and
I think it would make sense and it would be very convenient to make it
implicit if it fulfills the property 'dst_reg(src_reg(dst_reg(x))) ==
dst_reg(x)' and we fix it so the following code does the only one sane
thing:
| dst_reg reg = x;
| ADD(reg, src_reg(reg), y);
I can send patches to address the last two issues, actually I have a fix
for them lying around in some branch...
> Signed-off-by: Kenneth Graunke <kenn...@whitecape.org>
> ---
> src/mesa/drivers/dri/i965/brw_vec4.h | 6 +--
> src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp | 35 ++++++++-------
> src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 12 ++---
> src/mesa/drivers/dri/i965/gen6_gs_visitor.cpp | 55
> ++++++++++++-----------
> 4 files changed, 55 insertions(+), 53 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h
> b/src/mesa/drivers/dri/i965/brw_vec4.h
> index 8abd166..3d2882d 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4.h
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
> @@ -99,9 +99,9 @@ public:
>
> src_reg(register_file file, int reg, const glsl_type *type);
> src_reg();
> - src_reg(float f);
> - src_reg(uint32_t u);
> - src_reg(int32_t i);
> + explicit src_reg(float f);
> + explicit src_reg(uint32_t u);
> + explicit src_reg(int32_t i);
> src_reg(struct brw_reg reg);
>
> bool equals(const src_reg &r) const;
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
> b/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
> index db0e6cc..58c4df2 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
> @@ -150,7 +150,7 @@ vec4_gs_visitor::emit_prolog()
> */
> this->current_annotation = "clear r0.2";
> dst_reg r0(retype(brw_vec4_grf(0, 0), BRW_REGISTER_TYPE_UD));
> - vec4_instruction *inst = emit(GS_OPCODE_SET_DWORD_2, r0, 0u);
> + vec4_instruction *inst = emit(GS_OPCODE_SET_DWORD_2, r0, src_reg(0u));
> inst->force_writemask_all = true;
>
> /* Create a virtual register to hold the vertex count */
> @@ -158,7 +158,7 @@ vec4_gs_visitor::emit_prolog()
>
> /* Initialize the vertex_count register to 0 */
> this->current_annotation = "initialize vertex_count";
> - inst = emit(MOV(dst_reg(this->vertex_count), 0u));
> + inst = emit(MOV(dst_reg(this->vertex_count), src_reg(0u)));
> inst->force_writemask_all = true;
>
> if (c->control_data_header_size_bits > 0) {
> @@ -173,7 +173,7 @@ vec4_gs_visitor::emit_prolog()
> */
> if (c->control_data_header_size_bits <= 32) {
> this->current_annotation = "initialize control data bits";
> - inst = emit(MOV(dst_reg(this->control_data_bits), 0u));
> + inst = emit(MOV(dst_reg(this->control_data_bits), src_reg(0u)));
> inst->force_writemask_all = true;
> }
> }
> @@ -262,7 +262,7 @@ vec4_gs_visitor::emit_urb_write_header(int mrf)
> vec4_instruction *inst = emit(MOV(mrf_reg, r0));
> inst->force_writemask_all = true;
> emit(GS_OPCODE_SET_WRITE_OFFSET, mrf_reg, this->vertex_count,
> - (uint32_t) c->prog_data.output_vertex_size_hwords);
> + src_reg(uint32_t(c->prog_data.output_vertex_size_hwords)));
> }
>
>
> @@ -349,7 +349,7 @@ vec4_gs_visitor::emit_control_data_bits()
> /* If vertex_count is 0, then no control data bits have been accumulated
> * yet, so we should do nothing.
> */
> - emit(CMP(dst_null_d(), this->vertex_count, 0u, BRW_CONDITIONAL_NEQ));
> + emit(CMP(dst_null_d(), this->vertex_count, src_reg(0u),
> BRW_CONDITIONAL_NEQ));
> emit(IF(BRW_PREDICATE_NORMAL));
> {
> /* If we are using either channel masks or a per-slot offset, then we
> @@ -366,11 +366,12 @@ vec4_gs_visitor::emit_control_data_bits()
> src_reg dword_index(this, glsl_type::uint_type);
> if (urb_write_flags) {
> src_reg prev_count(this, glsl_type::uint_type);
> - emit(ADD(dst_reg(prev_count), this->vertex_count, 0xffffffffu));
> + emit(ADD(dst_reg(prev_count), this->vertex_count,
> + src_reg(0xffffffffu)));
> unsigned log2_bits_per_vertex =
> _mesa_fls(c->control_data_bits_per_vertex);
> emit(SHR(dst_reg(dword_index), prev_count,
> - (uint32_t) (6 - log2_bits_per_vertex)));
> + src_reg(uint32_t(6 - log2_bits_per_vertex))));
> }
>
> /* Start building the URB write message. The first MRF gets a copy of
> @@ -387,8 +388,8 @@ vec4_gs_visitor::emit_control_data_bits()
> * the appropriate OWORD within the control data header.
> */
> src_reg per_slot_offset(this, glsl_type::uint_type);
> - emit(SHR(dst_reg(per_slot_offset), dword_index, 2u));
> - emit(GS_OPCODE_SET_WRITE_OFFSET, mrf_reg, per_slot_offset, 1u);
> + emit(SHR(dst_reg(per_slot_offset), dword_index, src_reg(2u)));
> + emit(GS_OPCODE_SET_WRITE_OFFSET, mrf_reg, per_slot_offset,
> src_reg(1u));
> }
>
> if (urb_write_flags & BRW_URB_WRITE_USE_CHANNEL_MASKS) {
> @@ -400,10 +401,10 @@ vec4_gs_visitor::emit_control_data_bits()
> * together.
> */
> src_reg channel(this, glsl_type::uint_type);
> - inst = emit(AND(dst_reg(channel), dword_index, 3u));
> + inst = emit(AND(dst_reg(channel), dword_index, src_reg(3u)));
> inst->force_writemask_all = true;
> src_reg one(this, glsl_type::uint_type);
> - inst = emit(MOV(dst_reg(one), 1u));
> + inst = emit(MOV(dst_reg(one), src_reg(1u)));
> inst->force_writemask_all = true;
> src_reg channel_mask(this, glsl_type::uint_type);
> inst = emit(SHL(dst_reg(channel_mask), one, channel));
> @@ -455,11 +456,11 @@ vec4_gs_visitor::set_stream_control_data_bits(unsigned
> stream_id)
>
> /* reg::sid = stream_id */
> src_reg sid(this, glsl_type::uint_type);
> - emit(MOV(dst_reg(sid), stream_id));
> + emit(MOV(dst_reg(sid), src_reg(stream_id)));
>
> /* reg:shift_count = 2 * (vertex_count - 1) */
> src_reg shift_count(this, glsl_type::uint_type);
> - emit(SHL(dst_reg(shift_count), this->vertex_count, 1u));
> + emit(SHL(dst_reg(shift_count), this->vertex_count, src_reg(1u)));
>
> /* Note: we're relying on the fact that the GEN SHL instruction only pays
> * attention to the lower 5 bits of its second source argument, so on this
> @@ -514,7 +515,7 @@ vec4_gs_visitor::visit(ir_emit_vertex *ir)
> */
> vec4_instruction *inst =
> emit(AND(dst_null_d(), this->vertex_count,
> - (uint32_t) (32 / c->control_data_bits_per_vertex - 1)));
> + src_reg(uint32_t(32 / c->control_data_bits_per_vertex -
> 1))));
> inst->conditional_mod = BRW_CONDITIONAL_Z;
> emit(IF(BRW_PREDICATE_NORMAL));
> {
> @@ -527,7 +528,7 @@ vec4_gs_visitor::visit(ir_emit_vertex *ir)
> * effect of any call to EndPrimitive() that the shader may have
> * made before outputting its first vertex.
> */
> - inst = emit(MOV(dst_reg(this->control_data_bits), 0u));
> + inst = emit(MOV(dst_reg(this->control_data_bits), src_reg(0u)));
> inst->force_writemask_all = true;
> }
> emit(BRW_OPCODE_ENDIF);
> @@ -594,9 +595,9 @@ vec4_gs_visitor::visit(ir_end_primitive *)
>
> /* control_data_bits |= 1 << ((vertex_count - 1) % 32) */
> src_reg one(this, glsl_type::uint_type);
> - emit(MOV(dst_reg(one), 1u));
> + emit(MOV(dst_reg(one), src_reg(1u)));
> src_reg prev_count(this, glsl_type::uint_type);
> - emit(ADD(dst_reg(prev_count), this->vertex_count, 0xffffffffu));
> + emit(ADD(dst_reg(prev_count), this->vertex_count, src_reg(0xffffffffu)));
> src_reg mask(this, glsl_type::uint_type);
> /* Note: we're relying on the fact that the GEN SHL instruction only pays
> * attention to the lower 5 bits of its second source argument, so on this
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> index 8ce870c..c157c9b 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> @@ -2249,10 +2249,10 @@ vec4_visitor::visit_atomic_counter_intrinsic(ir_call
> *ir)
> deref_array->array_index->accept(this);
>
> src_reg tmp(this, glsl_type::uint_type);
> - emit(MUL(dst_reg(tmp), this->result, ATOMIC_COUNTER_SIZE));
> - emit(ADD(dst_reg(offset), tmp, location->data.atomic.offset));
> + emit(MUL(dst_reg(tmp), this->result, src_reg(ATOMIC_COUNTER_SIZE)));
> + emit(ADD(dst_reg(offset), tmp, src_reg(location->data.atomic.offset)));
> } else {
> - offset = location->data.atomic.offset;
> + offset = src_reg(location->data.atomic.offset);
> }
>
> /* Emit the appropriate machine instruction */
> @@ -2860,14 +2860,14 @@ vec4_visitor::emit_psiz_and_flags(dst_reg reg)
> dst_reg header1_w = header1;
> header1_w.writemask = WRITEMASK_W;
>
> - emit(MOV(header1, 0u));
> + emit(MOV(header1, src_reg(0u)));
>
> if (prog_data->vue_map.slots_valid & VARYING_BIT_PSIZ) {
> src_reg psiz = src_reg(output_reg[VARYING_SLOT_PSIZ]);
>
> current_annotation = "Point size";
> emit(MUL(header1_w, psiz, src_reg((float)(1 << 11))));
> - emit(AND(header1_w, src_reg(header1_w), 0x7ff << 8));
> + emit(AND(header1_w, src_reg(header1_w), src_reg(0x7ff << 8)));
> }
>
> if (key->userclip_active) {
> @@ -2907,7 +2907,7 @@ vec4_visitor::emit_psiz_and_flags(dst_reg reg)
>
> emit(MOV(retype(reg, BRW_REGISTER_TYPE_UD), src_reg(header1)));
> } else if (brw->gen < 6) {
> - emit(MOV(retype(reg, BRW_REGISTER_TYPE_UD), 0u));
> + emit(MOV(retype(reg, BRW_REGISTER_TYPE_UD), src_reg(0u)));
> } else {
> emit(MOV(retype(reg, BRW_REGISTER_TYPE_D), src_reg(0)));
> if (prog_data->vue_map.slots_valid & VARYING_BIT_PSIZ) {
> diff --git a/src/mesa/drivers/dri/i965/gen6_gs_visitor.cpp
> b/src/mesa/drivers/dri/i965/gen6_gs_visitor.cpp
> index d16cc6e..68906a7 100644
> --- a/src/mesa/drivers/dri/i965/gen6_gs_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/gen6_gs_visitor.cpp
> @@ -98,13 +98,13 @@ gen6_gs_visitor::emit_prolog()
> * headers.
> */
> this->first_vertex = src_reg(this, glsl_type::uint_type);
> - emit(MOV(dst_reg(this->first_vertex), URB_WRITE_PRIM_START));
> + emit(MOV(dst_reg(this->first_vertex), src_reg(URB_WRITE_PRIM_START)));
>
> /* The FF_SYNC message requires to know the number of primitives
> generated,
> * so keep a counter for this.
> */
> this->prim_count = src_reg(this, glsl_type::uint_type);
> - emit(MOV(dst_reg(this->prim_count), 0u));
> + emit(MOV(dst_reg(this->prim_count), src_reg(0u)));
>
> if (c->prog_data.gen6_xfb_enabled) {
> /* Create a virtual register to hold destination indices in SOL */
> @@ -188,7 +188,7 @@ gen6_gs_visitor::visit(ir_emit_vertex *)
> }
>
> emit(ADD(dst_reg(this->vertex_output_offset),
> - this->vertex_output_offset, 1u));
> + this->vertex_output_offset, src_reg(1u)));
> }
>
> /* Now buffer flags for this vertex */
> @@ -199,9 +199,9 @@ gen6_gs_visitor::visit(ir_emit_vertex *)
> /* If we are outputting points, then every vertex has PrimStart and
> * PrimEnd set.
> */
> - emit(MOV(dst, (_3DPRIM_POINTLIST << URB_WRITE_PRIM_TYPE_SHIFT) |
> - URB_WRITE_PRIM_START | URB_WRITE_PRIM_END));
> - emit(ADD(dst_reg(this->prim_count), this->prim_count, 1u));
> + emit(MOV(dst, src_reg(_3DPRIM_POINTLIST <<
> URB_WRITE_PRIM_TYPE_SHIFT |
> + URB_WRITE_PRIM_START | URB_WRITE_PRIM_END)));
> + emit(ADD(dst_reg(this->prim_count), this->prim_count, src_reg(1u)));
> } else {
> /* Otherwise, we can only set the PrimStart flag, which we have
> stored
> * in the first_vertex register. We will have to wait until we
> execute
> @@ -209,14 +209,15 @@ gen6_gs_visitor::visit(ir_emit_vertex *)
> * vertex.
> */
> emit(OR(dst, this->first_vertex,
> - (c->prog_data.output_topology <<
> URB_WRITE_PRIM_TYPE_SHIFT)));
> - emit(MOV(dst_reg(this->first_vertex), 0u));
> + src_reg(c->prog_data.output_topology <<
> + URB_WRITE_PRIM_TYPE_SHIFT)));
> + emit(MOV(dst_reg(this->first_vertex), src_reg(0u)));
> }
> emit(ADD(dst_reg(this->vertex_output_offset),
> - this->vertex_output_offset, 1u));
> + this->vertex_output_offset, src_reg(1u)));
>
> /* Update vertex count */
> - emit(ADD(dst_reg(this->vertex_count), this->vertex_count, 1u));
> + emit(ADD(dst_reg(this->vertex_count), this->vertex_count,
> src_reg(1u)));
> }
> emit(BRW_OPCODE_ENDIF);
> }
> @@ -244,7 +245,7 @@ gen6_gs_visitor::visit(ir_end_primitive *)
> emit(CMP(dst_null_d(), this->vertex_count, src_reg(num_output_vertices +
> 1),
> BRW_CONDITIONAL_L));
> vec4_instruction *inst = emit(CMP(dst_null_d(),
> - this->vertex_count, 0u,
> + this->vertex_count, src_reg(0u),
> BRW_CONDITIONAL_NEQ));
> inst->predicate = BRW_PREDICATE_NORMAL;
> emit(IF(BRW_PREDICATE_NORMAL));
> @@ -260,13 +261,13 @@ gen6_gs_visitor::visit(ir_end_primitive *)
> dst.reladdr = ralloc(mem_ctx, src_reg);
> memcpy(dst.reladdr, &offset, sizeof(src_reg));
>
> - emit(OR(dst_reg(dst), dst, URB_WRITE_PRIM_END));
> - emit(ADD(dst_reg(this->prim_count), this->prim_count, 1u));
> + emit(OR(dst_reg(dst), dst, src_reg(URB_WRITE_PRIM_END)));
> + emit(ADD(dst_reg(this->prim_count), this->prim_count, src_reg(1u)));
>
> /* Set the first vertex flag to indicate that the next vertex will
> start
> * a primitive.
> */
> - emit(MOV(dst_reg(this->first_vertex), URB_WRITE_PRIM_START));
> + emit(MOV(dst_reg(this->first_vertex), src_reg(URB_WRITE_PRIM_START)));
> }
> emit(BRW_OPCODE_ENDIF);
> }
> @@ -339,7 +340,7 @@ gen6_gs_visitor::emit_thread_end()
> * points because in the point case we set PrimEnd on all vertices.
> */
> if (c->gp->program.OutputType != GL_POINTS) {
> - emit(CMP(dst_null_d(), this->first_vertex, 0u, BRW_CONDITIONAL_Z));
> + emit(CMP(dst_null_d(), this->first_vertex, src_reg(0u),
> BRW_CONDITIONAL_Z));
> emit(IF(BRW_PREDICATE_NORMAL));
> {
> visit((ir_end_primitive *) NULL);
> @@ -367,7 +368,7 @@ gen6_gs_visitor::emit_thread_end()
> int max_usable_mrf = 13;
>
> /* Issue the FF_SYNC message and obtain the initial VUE handle. */
> - emit(CMP(dst_null_d(), this->vertex_count, 0u, BRW_CONDITIONAL_G));
> + emit(CMP(dst_null_d(), this->vertex_count, src_reg(0u),
> BRW_CONDITIONAL_G));
> emit(IF(BRW_PREDICATE_NORMAL));
> {
> this->current_annotation = "gen6 thread end: ff_sync";
> @@ -391,8 +392,8 @@ gen6_gs_visitor::emit_thread_end()
> /* Loop over all buffered vertices and emit URB write messages */
> this->current_annotation = "gen6 thread end: urb writes init";
> src_reg vertex(this, glsl_type::uint_type);
> - emit(MOV(dst_reg(vertex), 0u));
> - emit(MOV(dst_reg(this->vertex_output_offset), 0u));
> + emit(MOV(dst_reg(vertex), src_reg(0u)));
> + emit(MOV(dst_reg(this->vertex_output_offset), src_reg(0u)));
>
> this->current_annotation = "gen6 thread end: urb writes";
> emit(BRW_OPCODE_DO);
> @@ -436,7 +437,7 @@ gen6_gs_visitor::emit_thread_end()
>
> mrf++;
> emit(ADD(dst_reg(this->vertex_output_offset),
> - this->vertex_output_offset, 1u));
> + this->vertex_output_offset, src_reg(1u)));
>
> /* If this was max_usable_mrf, we can't fit anything more into
> * this URB WRITE.
> @@ -456,9 +457,9 @@ gen6_gs_visitor::emit_thread_end()
> * writing the next vertex.
> */
> emit(ADD(dst_reg(this->vertex_output_offset),
> - this->vertex_output_offset, 1u));
> + this->vertex_output_offset, src_reg(1u)));
>
> - emit(ADD(dst_reg(vertex), vertex, 1u));
> + emit(ADD(dst_reg(vertex), vertex, src_reg(1u)));
> }
> emit(BRW_OPCODE_WHILE);
>
> @@ -612,8 +613,8 @@ gen6_gs_visitor::xfb_write()
>
> this->current_annotation = "gen6 thread end: svb writes init";
>
> - emit(MOV(dst_reg(this->vertex_output_offset), 0u));
> - emit(MOV(dst_reg(this->sol_prim_written), 0u));
> + emit(MOV(dst_reg(this->vertex_output_offset), src_reg(0u)));
> + emit(MOV(dst_reg(this->sol_prim_written), src_reg(0u)));
>
> /* Check that at least one primitive can be written
> *
> @@ -647,7 +648,7 @@ gen6_gs_visitor::xfb_write()
>
> /* Write transform feedback data for all processed vertices. */
> for (int i = 0; i < c->gp->program.VerticesOut; i++) {
> - emit(MOV(dst_reg(sol_temp), i));
> + emit(MOV(dst_reg(sol_temp), src_reg(i)));
> emit(CMP(dst_null_d(), sol_temp, this->vertex_count,
> BRW_CONDITIONAL_L));
> emit(IF(BRW_PREDICATE_NORMAL));
> @@ -670,7 +671,7 @@ gen6_gs_visitor::xfb_program(unsigned vertex, unsigned
> num_verts)
> /* Check for buffer overflow: we need room to write the complete primitive
> * (all vertices). Otherwise, avoid writing any vertices for it
> */
> - emit(ADD(dst_reg(sol_temp), this->sol_prim_written, 1u));
> + emit(ADD(dst_reg(sol_temp), this->sol_prim_written, src_reg(1u)));
> emit(MUL(dst_reg(sol_temp), sol_temp, brw_imm_ud(num_verts)));
> emit(ADD(dst_reg(sol_temp), sol_temp, this->svbi));
> emit(CMP(dst_null_d(), sol_temp, this->max_svbi, BRW_CONDITIONAL_LE));
> @@ -709,7 +710,7 @@ gen6_gs_visitor::xfb_program(unsigned vertex, unsigned
> num_verts)
> src_reg data(this->vertex_output);
> data.reladdr = ralloc(mem_ctx, src_reg);
> int offset = get_vertex_output_offset_for_varying(vertex, varying);
> - emit(MOV(dst_reg(this->vertex_output_offset), offset));
> + emit(MOV(dst_reg(this->vertex_output_offset), src_reg(offset)));
> memcpy(data.reladdr, &this->vertex_output_offset, sizeof(src_reg));
> data.type = output_reg[varying].type;
>
> @@ -738,7 +739,7 @@ gen6_gs_visitor::xfb_program(unsigned vertex, unsigned
> num_verts)
> this->destination_indices,
> brw_imm_ud(num_verts)));
> emit(ADD(dst_reg(this->sol_prim_written),
> - this->sol_prim_written, 1u));
> + this->sol_prim_written, src_reg(1u)));
> }
>
> }
> --
> 2.1.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
pgpGFhvzlkmgB.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
