<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, May 27, 2016 at 1:16 PM, Jason Ekstrand <span dir="ltr"><<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>></span> wrote:<br><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><div class="gmail_quote"><div><div>On Fri, May 27, 2016 at 1:10 PM, Francisco Jerez <span dir="ltr"><<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> writes:<br> <br> > On Thu, May 26, 2016 at 8:46 PM, Francisco Jerez <<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>><br> > wrote:<br> ><br> >> ---<br> >> src/mesa/drivers/dri/i965/brw_fs.cpp | 26 ++++++++++++++++++++++++++<br> >> 1 file changed, 26 insertions(+)<br> >><br> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp<br> >> b/src/mesa/drivers/dri/i965/brw_fs.cpp<br> >> index 1f3b23b..7002346 100644<br> >> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp<br> >> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp<br> >> @@ -6547,6 +6547,32 @@ brw_compile_cs(const struct brw_compiler *compiler,<br> >> void *log_data,<br> >> }<br> >> }<br> >><br> >> + fs_visitor v32(compiler, log_data, mem_ctx, key, &prog_data->base,<br> >> + NULL, /* Never used in core profile */<br> >> + shader, 32, shader_time_index);<br> >> + if (!fail_msg && v8.max_dispatch_width >= 32 &&<br> >> + simd_required == 32) {<br> >><br> ><br> > I don't see where simd_required is getting aligned up to a power-of-two so<br> > how are we expecting to hit this? Also, I took a look at the SIMD16 case<br> > above, and we're hand-rolling simd_required there (which we shouldn't be).<br> > Here's what I would suggest:<br> ><br> <br> </span>Yeah, I had noticed that last night shortly after sending, the version<br> of this patch I pushed in the branch shouldn't have this bug:<br> <br> <a href="https://cgit.freedesktop.org/~currojerez/mesa/commit/?h=i965-simd32-cs&id=3d7f22ffd567901c9561b93b0d6945a50a095997" rel="noreferrer" target="_blank">https://cgit.freedesktop.org/~currojerez/mesa/commit/?h=i965-simd32-cs&id=3d7f22ffd567901c9561b93b0d6945a50a095997</a><br> <span><br> > simd_required = DIV_ROUND_UP(...)<br> > min_simd = 32<br> ><br> > then, in each one we do<br> ><br> > if ((INTEL_DEBUG & DEBUG_NO16) && simd_required <= 16 && min_simd >= 16) {<br> > if (min_simd == 8)<br> > v16.import_uniforms(v8)<br> > if (!v16.run_cs()) {<br> > /* fail */<br> > } else {<br> > /* success */<br> > min_simd = MIN2(min_simd, 16);<br> > }<br> > }<br> ><br> > with the obvious adjustments for 8 and 32. That way no8 and no16 both work<br> > fine and we properly guarantee that we compile exactly one shader.<br> ><br> <br> </span>So if I'm understanding correctly you're asking me to rework the SIMD16<br> and SIMD8 back-end invocation so the no8 and no16 debugging options are<br> taken into account for compute shaders? That would definitely make<br> PATCH 23 unnecessary but will be somewhat more churn (not saying that it<br> wouldn't make sense to rewrite brw_compile_cs() eventually).<br></blockquote><div><br></div></div></div><div>Well, we already take no16 into account and the uniform import code here (and in the patch above) won't work if you use INTEL_DEBUG=no16 and need more than 8 because it's using simd_required for both the minimum and maximum. In other words, I think it's horribly broken and we should fix it. If you don't want to fix it now for the sake of time, I understand, but we should back-port the fix so that it's not broken in 12.0.<span><font color="#888888"><br></font></span></div></div></div></div></blockquote><div><br></div><div>I looked a bit harder at this and it's more complex than I first thought. Other than the obvious simd_required fix, I'm not 100% sure what it should look like. Go ahead and just push the version in your branch.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><span><font color="#888888"></font></span></div><span><font color="#888888"><div>--Jason<br></div></font></span><div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div> > Seem reasonable?<br> ><br> > --Jason<br> ><br> ><br> >> + /* Try a SIMD32 compile */<br> >> + if (simd_required <= 8)<br> >> + v32.import_uniforms(&v8);<br> >> + else if (simd_required <= 16)<br> >> + v32.import_uniforms(&v16);<br> >> +<br> >> + if (!v32.run_cs()) {<br> >> + compiler->shader_perf_log(log_data,<br> >> + "SIMD32 shader failed to compile: %s",<br> >> + v16.fail_msg);<br> >> + if (!cfg) {<br> >> + fail_msg =<br> >> + "Couldn't generate SIMD32 program and not "<br> >> + "enough threads for SIMD16";<br> >> + }<br> >> + } else {<br> >> + cfg = v32.cfg;<br> >> + prog_data->simd_size = 32;<br> >> + }<br> >> + }<br> >> +<br> >> if (unlikely(cfg == NULL)) {<br> >> assert(fail_msg);<br> >> if (error_str)<br> >> --<br> >> 2.7.3<br> >><br> >> _______________________________________________<br> >> mesa-dev mailing list<br> >> <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br> >> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br> >><br> </div></div></blockquote></div></div></div><br></div></div> </blockquote></div><br></div></div>