<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jun 24, 2014 at 10:23 AM, Kenneth Graunke <span dir="ltr"><<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tuesday, June 24, 2014 08:17:23 AM Ian Romanick wrote:<br>
> Send patches with git-send-email and no other method. Save this message<br>
> and apply it to origin/master using git-am to see why.<br>
><br>
> Hopefully Matt or Ken can review this...<br>
><br>
> On 06/12/2014 12:35 PM, Cody Northrop wrote:<br>
> > Commit 17c7ead7 exposed a bug in how uniform loading happens in the<br>
> > presence of discard. It manifested itself in an application as randomly<br>
> > incorrect pixels on the borders of conditional areas.<br>
> ><br>
> > We believe it is due to how discards jump to the end of the shader<br>
> > incorrectly for some channels. The current implementation checks each<br>
> > 2x2 subspan to preserve derivatives. When uniform loading via samplers<br>
> > was turned on, it uses a full execution mask, as stated in<br>
> > lower_uniform_pull_constant_loads(), and only populates four channels of<br>
> > the destination (see generate_uniform_pull_constant_load_gen7()). We<br>
> > think that is happening incorrectly when the first subspan has been<br>
> > jumped over.<br>
> ><br>
> > A possible fix is to only jump to the end of the shader if all relevant<br>
> > channels are disabled, i.e. all 8 or 16, depending on dispatch. This<br>
> > preserves the original speedup noted in commit beafced2. There are<br>
> > other more heavyweight fixes (i.e. don't use sampler for uniforms if<br>
> > discard in use), but this seems like a good fix. I've appended it below<br>
> > as a patch. It changes the shader accordingly:<br>
> ><br>
> > before : 0x000000d0: (-f0.1.any4h) halt(8) 17 2<br>
> > null { align1 WE_all 1Q };<br>
> > after(8) : 0x000000d0: (-f0.1.any8h) halt(8) 17 2<br>
> > null { align1 WE_all 1Q };<br>
> > after(16) : 0x00000250: (-f0.1.any16h) halt(16) 17 2<br>
> > null { align1 WE_all 1H };<br>
><br>
> All of this information should go in the commit message.<br>
><br>
> > I attached a test case to the bugzilla entry below.<br>
><br>
> Was the test also sent to the piglit mailing list for inclusion in the<br>
> the test suite?<br>
><br>
> > Thanks for any review or feedback. Curious if anyone sees a better fix.<br>
> ><br>
> > -C<br>
> ><br>
> ><br>
> > From 871671c4ab8ff00e85b434865e8855fc356efa8f Mon Sep 17 00:00:00 2001<br>
> > From: Cody Northrop <<a href="mailto:cody@lunarg.com">cody@lunarg.com</a> <mailto:<a href="mailto:cody@lunarg.com">cody@lunarg.com</a>>><br>
> > Date: Thu, 12 Jun 2014 09:07:18 -0600<br>
> > Subject: [PATCH] i965/fs: Update discard jump to preserve uniform loads<br>
via<br>
> > sampler.<br>
> ><br>
> > The series that implemented this optimization was done before<br>
> > the changes to use samplers for uniform loads. Uniform sampler<br>
> > loads use special execution masks and only populate four<br>
> > channels, so we can't jump over those or corruption ensues.<br>
> > Use a more conservative jump mask which only jumps to the end<br>
> > if all relevant channels are disabled.<br>
> ><br>
> > No change was observed in GLbenchmark 2.7, so the optimization<br>
> > is preserved.<br>
> ><br>
> > Signed-off-by: Cody Northrop <<a href="mailto:cody@lunarg.com">cody@lunarg.com</a> <mailto:<a href="mailto:cody@lunarg.com">cody@lunarg.com</a>>><br>
> > Reviewed-by: Mike Stroyan <<a href="mailto:mike@lunarg.com">mike@lunarg.com</a> <mailto:<a href="mailto:mike@lunarg.com">mike@lunarg.com</a>>><br>
> > Bugzilla: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=79948" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=79948</a><br>
> > ---<br>
> > src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 10 +++++++++-<br>
> > 1 file changed, 9 insertions(+), 1 deletion(-)<br>
> ><br>
> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp<br>
> > b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp<br>
> > index 8858852..fe05715 100644<br>
> > --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp<br>
> > +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp<br>
> > @@ -1907,7 +1907,15 @@ fs_visitor::visit(ir_discard *ir)<br>
> > */<br>
> > fs_inst *discard_jump = emit(FS_OPCODE_DISCARD_JUMP);<br>
> > discard_jump->flag_subreg = 1;<br>
> > - discard_jump->predicate = BRW_PREDICATE_ALIGN1_ANY4H;<br>
> > +<br>
> > + /* Uniforms are now loaded using samplers with a routine that has<br>
> > + * its own execution mask, so we can only jump if all relevant<br>
> > + * channels are dead. This is more conservative than the previous<br>
> > + * four channel checking, but still preserves speedups.<br>
<br>
</div></div>This comment won't make much sense when reading the code, since "the previous<br>
four channel checking" no longer exists and can't be observed anywhere other<br>
than in git history. Personally, I'd just drop the comment.<br>
<br></blockquote><div><br></div><div>Sure, I'll clean up the comments.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
As far as I can tell, this is just broken and should have always been<br>
ANY8H/ANY16H. Good catch, and thanks for fixing it!<br>
<div class=""><br>
> > + */<br>
> > + discard_jump->predicate = (8 == dispatch_width)<br>
<br>
</div>Please drop the Yoda-conditional and use (dispatch_width == 8).<br></blockquote><div><br></div><div>Since dispatch_width is constant, it can go on the left. '8' was more read-only to my eyes. I try to avoid common typo mistakes (= instead of ==).<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
With those two changes, this would get:<br>
Reviewed-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>><br>
<br>
If you resend a v2 using git-send-email, I'll be happy to push it.<br></blockquote><div><br></div><div>Thanks, I'll fix all of this and send again correctly.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
> > + ? BRW_PREDICATE_ALIGN1_ANY8H<br>
> > + : BRW_PREDICATE_ALIGN1_ANY16H;<br>
> > discard_jump->predicate_inverse = true;<br>
> > }<br>
> > }</div></div></blockquote></div><br><br clear="all"><br>-- <br><div dir="ltr"><div><font color="#999999"><font face="trebuchet ms, sans-serif"> Cody Northrop<br> Graphics Software Engineer<br> LunarG, Inc.- 3D Driver Innovations</font><font style="font-size:small" face="trebuchet ms, sans-serif"><br>
Email: <a href="mailto:cody@lunarg.com" target="_blank">cody@lunarg.com</a><br> Website: <a href="http://www.lunarg.com/" target="_blank">http://www.lunarg.com</a></font></font></div></div>
</div></div>