<div dir="ltr">On 9 August 2013 17:14, Chris Forbes <span dir="ltr"><<a href="mailto:chrisf@ijw.co.nz" target="_blank">chrisf@ijw.co.nz</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
V2: - Use the new VS_OPCODE_UNPACK_FLAGS_SIMD4X2 to correctly split the<br>
flags for the two vertices being processed together.<br>
- Don't apply bogus masking of clip flags. The set of plane enables<br>
aren't included in the shader key, and we wouldn't want the<br>
recompiles anyway.<br>
<br>
Signed-off-by: Chris Forbes <<a href="mailto:chrisf@ijw.co.nz">chrisf@ijw.co.nz</a>><br>
---<br>
src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 25 ++++++++++++++-----------<br>
1 file changed, 14 insertions(+), 11 deletions(-)<br>
<br>
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp<br>
index f80777b..5e007de 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp<br>
+++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp<br>
@@ -2625,7 +2625,6 @@ vec4_visitor::emit_psiz_and_flags(struct brw_reg reg)<br>
dst_reg header1 = dst_reg(this, glsl_type::uvec4_type);<br>
dst_reg header1_w = header1;<br>
header1_w.writemask = WRITEMASK_W;<br>
- GLuint i;<br>
<br>
emit(MOV(header1, 0u));<br>
<br>
@@ -2637,18 +2636,22 @@ vec4_visitor::emit_psiz_and_flags(struct brw_reg reg)<br>
emit(AND(header1_w, src_reg(header1_w), 0x7ff << 8));<br>
}<br>
<br>
- current_annotation = "Clipping flags";<br>
- for (i = 0; i < key->nr_userclip_plane_consts; i++) {<br>
- vec4_instruction *inst;<br>
- gl_varying_slot slot = (prog_data->vue_map.slots_valid & VARYING_BIT_CLIP_VERTEX)<br>
- ? VARYING_SLOT_CLIP_VERTEX : VARYING_SLOT_POS;<br>
+ if (key->userclip_active) {<br>
+ current_annotation = "Clipping flags";<br>
+ dst_reg temp = dst_reg(this, glsl_type::uint_type);<br>
+ dst_reg temp2 = dst_reg(this, glsl_type::uint_type);<br>
<br>
- inst = emit(DP4(dst_null_f(), src_reg(output_reg[slot]),<br>
- src_reg(this->userplane[i])));<br>
- inst->conditional_mod = BRW_CONDITIONAL_L;<br>
+ emit(CMP(dst_null_f(), src_reg(output_reg[VARYING_SLOT_CLIP_DIST0]), src_reg(0.0f), BRW_CONDITIONAL_L));<br>
+ emit(VS_OPCODE_UNPACK_FLAGS_SIMD4X2, temp, src_reg(0));<br>
+ emit(AND(temp2, src_reg(temp), src_reg(0x0fu)));<br></blockquote><div><br></div><div>I think this AND is unnecessary. VS_OPCODE_UNPACK_FLAGS_SIMD4X2 is guaranteed to produce outputs that only have bits 0-3 set.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
- inst = emit(OR(header1_w, src_reg(header1_w), 1u << i));<br>
- inst->predicate = BRW_PREDICATE_NORMAL;<br>
+ emit(CMP(dst_null_f(), src_reg(output_reg[VARYING_SLOT_CLIP_DIST1]), src_reg(0.0f), BRW_CONDITIONAL_L));<br>
+ emit(VS_OPCODE_UNPACK_FLAGS_SIMD4X2, temp, src_reg(0));<br>
+ emit(AND(temp, src_reg(temp), src_reg(0x0fu)));<br></blockquote><div><br></div><div>Same with this AND.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ emit(SHL(temp, src_reg(temp), src_reg(4)));<br>
+ emit(OR(temp2, src_reg(temp2), src_reg(temp)));<br>
+<br>
+ emit(OR(header1_w, src_reg(header1_w), src_reg(temp2)));<br></blockquote><div><br></div><div>Slight nit-pick: re-using temp registers introduces extra dependencies, making it more difficult for the instruction scheduler to produce efficient code. It probably won't produce a significant performance difference, but you might want to consider using a separate temp register for each intermediate result. It shouldn't create a sizeable amount of extra register pressure since this code executes at the end ofthe shader anyhow.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
}<br>
<br>
/* i965 clipping workaround:<br>
<span class="HOEnZb"><font color="#888888">--<br>
1.8.3.4<br></font></span></blockquote><div><br></div><div>All of my suggestions on this patch are minor, so whether or not you decide to follow them, this patch is:<br><br>Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>><br>
</div></div></div></div>