<div dir="ltr">On 29 October 2013 19:22, Anuj Phogat <span dir="ltr"><<a href="mailto:anuj.phogat@gmail.com" target="_blank">anuj.phogat@gmail.com</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"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div><div class="h5">On Mon, Oct 28, 2013 at 5:10 PM, Paul Berry <span dir="ltr"><<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div>On 25 October 2013 16:45, Anuj Phogat <span dir="ltr"><<a href="mailto:anuj.phogat@gmail.com" target="_blank">anuj.phogat@gmail.com</a>></span> wrote:<br>
</div></div><div class="gmail_extra"><div class="gmail_quote"><div><div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">V2:<br>
- Update comments<br>
- Use fs_reg(0xffff) in AND instruction to get the 16 bit<br>
sample_mask.<br>
- Add a special backend instructions to compute sample_mask.<br>
- Add a new variable uses_omask in brw_wm_prog_data.<br>
<br>
Signed-off-by: Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com" target="_blank">anuj.phogat@gmail.com</a>><br>
---<br>
src/mesa/drivers/dri/i965/brw_context.h | 1 +<br>
src/mesa/drivers/dri/i965/brw_defines.h | 1 +<br>
src/mesa/drivers/dri/i965/brw_fs.h | 5 +++++<br>
src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 23 +++++++++++++++++++++++<br>
src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 15 +++++++++++++++<br>
5 files changed, 45 insertions(+)<br>
<br>
diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h<br>
index d16f257..d623368 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_context.h<br>
+++ b/src/mesa/drivers/dri/i965/brw_context.h<br>
@@ -381,6 +381,7 @@ struct brw_wm_prog_data {<br>
GLuint nr_pull_params;<br>
bool dual_src_blend;<br>
bool uses_pos_offset;<br>
+ bool uses_omask;<br>
uint32_t prog_offset_16;<br>
<br>
/**<br>
diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h<br>
index f3c994b..f9556a5 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_defines.h<br>
+++ b/src/mesa/drivers/dri/i965/brw_defines.h<br>
@@ -788,6 +788,7 @@ enum opcode {<br>
FS_OPCODE_VARYING_PULL_CONSTANT_LOAD_GEN7,<br>
FS_OPCODE_MOV_DISPATCH_TO_FLAGS,<br>
FS_OPCODE_DISCARD_JUMP,<br>
+ FS_OPCODE_SET_OMASK,<br>
FS_OPCODE_SET_SAMPLE_ID,<br>
FS_OPCODE_SET_SIMD4X2_OFFSET,<br>
FS_OPCODE_PACK_HALF_2x16_SPLIT,<br>
diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h<br>
index 8a1a414..c9bcc4e 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_fs.h<br>
+++ b/src/mesa/drivers/dri/i965/brw_fs.h<br>
@@ -436,6 +436,7 @@ public:<br>
<br>
struct hash_table *variable_ht;<br>
fs_reg frag_depth;<br>
+ fs_reg sample_mask;<br>
fs_reg outputs[BRW_MAX_DRAW_BUFFERS];<br>
unsigned output_components[BRW_MAX_DRAW_BUFFERS];<br>
fs_reg dual_src_output;<br>
@@ -540,6 +541,10 @@ private:<br>
struct brw_reg offset);<br>
void generate_mov_dispatch_to_flags(fs_inst *inst);<br>
<br>
+ void generate_set_omask(fs_inst *inst,<br>
+ struct brw_reg dst,<br>
+ struct brw_reg sample_mask);<br>
+<br>
void generate_set_sample_id(fs_inst *inst,<br>
struct brw_reg dst,<br>
struct brw_reg src0,<br>
diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp<br>
index 4f15ed7..fc8e0bd 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp<br>
+++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp<br>
@@ -1024,6 +1024,25 @@ fs_generator::generate_set_simd4x2_offset(fs_inst *inst,<br>
brw_pop_insn_state(p);<br>
}<br>
<br>
+/* Sets vstride=16, width=8, hstride=2 of register mask before<br>
+ * moving to register dst.<br>
+ */<br>
+void<br>
+fs_generator::generate_set_omask(fs_inst *inst,<br>
+ struct brw_reg dst,<br>
+ struct brw_reg mask)<br>
+{<br>
+ assert(dst.type == BRW_REGISTER_TYPE_UW);<br>
+ if (dispatch_width == 16)<br>
+ dst = vec16(dst);<br>
+ brw_push_insn_state(p);<br>
+ brw_set_compression_control(p, BRW_COMPRESSION_NONE);<br>
+ brw_set_mask_control(p, BRW_MASK_DISABLE);<br>
+ brw_MOV(p, dst, stride(retype(brw_vec1_reg(mask.file, <a href="http://mask.nr" target="_blank">mask.nr</a>, 0),<br>
+ dst.type), 16, 8, 2));<br>
+ brw_pop_insn_state(p);<br>
+}<br>
+<br>
/* Sets vstride=1, width=4, hstride=0 of register src1 during<br>
* the ADD instruction.<br>
*/<br>
@@ -1576,6 +1595,10 @@ fs_generator::generate_code(exec_list *instructions)<br>
generate_set_simd4x2_offset(inst, dst, src[0]);<br>
break;<br>
<br>
+ case FS_OPCODE_SET_OMASK:<br>
+ generate_set_omask(inst, dst, src[0]);<br>
+ break;<br>
+<br>
case FS_OPCODE_SET_SAMPLE_ID:<br>
generate_set_sample_id(inst, dst, src[0], src[1]);<br>
break;<br>
diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp<br>
index 7a6a0b5..b9eb5b8 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp<br>
+++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp<br>
@@ -82,6 +82,8 @@ fs_visitor::visit(ir_variable *ir)<br>
}<br>
} else if (ir->location == FRAG_RESULT_DEPTH) {<br>
this->frag_depth = *reg;<br>
+ } else if (ir->location == FRAG_RESULT_SAMPLE_MASK) {<br>
+ this->sample_mask = *reg;<br>
} else {<br>
/* gl_FragData or a user-defined FS output */<br>
assert(ir->location >= FRAG_RESULT_DATA0 &&<br>
@@ -2510,6 +2512,19 @@ fs_visitor::emit_fb_writes()<br>
pop_force_uncompressed();<br>
}<br>
<br>
+ c->prog_data.uses_omask =<br>
+ fp->Base.OutputsWritten & BITFIELD64_BIT(FRAG_RESULT_SAMPLE_MASK);<br>
+ if(c->prog_data.uses_omask) {<br>
+ this->current_annotation = "FB write oMask";<br>
+ assert(this->sample_mask.file != BAD_FILE);<br>
+ fs_reg reg = fs_reg(this, glsl_type::int_type);<br>
+<br>
+ /* Hand over gl_SampleMask. Only lower 16 bits are relevant. */<br>
+ emit(AND(reg, this->sample_mask, fs_reg(0xffff)));<br></blockquote><div><br></div></div></div><div>I think we can drop this AND instruction, since FS_OPCODE_SET_OMASK wll treat reg as UW, and it will use a stride that causes it to skip alternate words. So the upper 16 bits of sample mask are ignored anyway.<br>
</div></div></div></div></blockquote></div></div><div>Dropping the AND instruction works fine if we're doing something like this in FS:</div><div>out vec4 out_color;<br></div><div>main() </div><div>{</div>
<div> gl_SampleMask[0] = 0x1234;</div><div> out_color = vec4(0.0, 1.0, 0.0, 1.0); </div><div>}</div><div>Relevant instructions generated for sample mask are:</div><div>mov(16) g2<1>D 0x1234<br>
</div><div>mov(16) g113<1>UW g2<16,8,2>UW<br></div><div><br></div><div>But if we do this in FS:</div><div>out vec4 out_color;<br></div><div>uniform int mask;</div><div>
main()</div><div>{</div><div><div> gl_SampleMask[0] = mask;</div><div> out_color = vec4(0.0, 1.0, 0.0, 1.0); </div></div><div>}</div><div><br></div><div>Relevant instructions generated for sample mask are:<br>
</div><div> mov(16) g113<1>UW g2<16,8,2>UW<br></div><div><br></div><div>It doesn't work because uniform values are passed in push constants. So, this case require a different stride of g2<0,1,0>. <br>
</div><div>If we use AND instruction both of above cases can use same stride of g2<16,8,2>. Using MOV in place of AND doesn't help either because the MOV instruction gets optimized away. We may need a new backend instruction to make the MOV work. But this doesn't seem worth avoiding an AND instruction. Please correct me if I'm missing something here.</div>
</div></div></div></blockquote><div><br></div><div>Oh, wow. I didn't think about that case. Good catch.<br><br></div><div>I have one last idea, and then I'll drop the subject. What if, instead of making a new backend instruction to make the MOV work, we just modify generate_set_omask() so that it examines the stride of the incoming mask register. If it's <8;8,1> (an ordinary SIMD8 or SIMD16 register), it modifies it to <16;8,2>. If it's <0;1,0> (a uniform), it leaves it as <0;1,0>. Would that work?<br>
<br></div><div>Assuming that it works, I have a preference for doing it that way, partly because it saves an instruction, and partly because it seems a little fragile to me to use an AND instruction to work around the fs_generator not handling a uniform properly.<br>
<br>But I don't want to get in the way of progress. So either way, consider the patch:<br><br></div><div>Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>><br></div><div>
<br></div><div>Thanks for being willing to follow up on all these small details, Anuj.<br></div></div></div></div>