Re: [Mesa-dev] [PATCH 4/4] i965/vec4: Make src_reg immediate constructors explicit.
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 .
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),
[Mesa-dev] [PATCH 4/4] i965/vec4: Make src_reg immediate constructors explicit.
We did this for fs_reg a while back, and it's generally a good idea.
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, 0xu));
+ emit(ADD(dst_reg(prev_count), this-vertex_count,
+ src_reg(0xu)));
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));
+
