[Mesa-dev] [PATCH 22/24] i965/fs: Build 32-wide compute shader when needed.

[Jason Ekstrand]

On Fri, May 27, 2016 at 1:10 PM, Francisco Jerez <currojerez at riseup.net>
wrote:

> Jason Ekstrand <jason at jlekstrand.net> writes:
>
> > On Thu, May 26, 2016 at 8:46 PM, Francisco Jerez <currojerez at riseup.net>
> > wrote:
> >
> >> ---
> >>  src/mesa/drivers/dri/i965/brw_fs.cpp | 26 ++++++++++++++++++++++++++
> >>  1 file changed, 26 insertions(+)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> b/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> index 1f3b23b..7002346 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> @@ -6547,6 +6547,32 @@ brw_compile_cs(const struct brw_compiler
> *compiler,
> >> void *log_data,
> >>        }
> >>     }
> >>
> >> +   fs_visitor v32(compiler, log_data, mem_ctx, key, &prog_data->base,
> >> +                 NULL, /* Never used in core profile */
> >> +                 shader, 32, shader_time_index);
> >> +   if (!fail_msg && v8.max_dispatch_width >= 32 &&
> >> +       simd_required == 32) {
> >>
> >
> > I don't see where simd_required is getting aligned up to a power-of-two
> so
> > how are we expecting to hit this?  Also, I took a look at the SIMD16 case
> > above, and we're hand-rolling simd_required there (which we shouldn't
> be).
> > Here's what I would suggest:
> >
>
> Yeah, I had noticed that last night shortly after sending, the version
> of this patch I pushed in the branch shouldn't have this bug:
>
>
> https://cgit.freedesktop.org/~currojerez/mesa/commit/?h=i965-simd32-cs&id=3d7f22ffd567901c9561b93b0d6945a50a095997
>
> > simd_required = DIV_ROUND_UP(...)
> > min_simd = 32
> >
> > then, in each one we do
> >
> > if ((INTEL_DEBUG & DEBUG_NO16) && simd_required <= 16 && min_simd >= 16)
> {
> >     if (min_simd == 8)
> >         v16.import_uniforms(v8)
> >     if (!v16.run_cs()) {
> >         /* fail */
> >     } else {
> >         /* success */
> >         min_simd = MIN2(min_simd, 16);
> >     }
> > }
> >
> > with the obvious adjustments for 8 and 32.  That way no8 and no16 both
> work
> > fine and we properly guarantee that we compile exactly one shader.
> >
>
> So if I'm understanding correctly you're asking me to rework the SIMD16
> and SIMD8 back-end invocation so the no8 and no16 debugging options are
> taken into account for compute shaders?  That would definitely make
> PATCH 23 unnecessary but will be somewhat more churn (not saying that it
> wouldn't make sense to rewrite brw_compile_cs() eventually).
>

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.
--Jason


> > Seem reasonable?
> >
> > --Jason
> >
> >
> >> +      /* Try a SIMD32 compile */
> >> +      if (simd_required <= 8)
> >> +         v32.import_uniforms(&v8);
> >> +      else if (simd_required <= 16)
> >> +         v32.import_uniforms(&v16);
> >> +
> >> +      if (!v32.run_cs()) {
> >> +         compiler->shader_perf_log(log_data,
> >> +                                   "SIMD32 shader failed to compile:
> %s",
> >> +                                   v16.fail_msg);
> >> +         if (!cfg) {
> >> +            fail_msg =
> >> +               "Couldn't generate SIMD32 program and not "
> >> +               "enough threads for SIMD16";
> >> +         }
> >> +      } else {
> >> +         cfg = v32.cfg;
> >> +         prog_data->simd_size = 32;
> >> +      }
> >> +   }
> >> +
> >>     if (unlikely(cfg == NULL)) {
> >>        assert(fail_msg);
> >>        if (error_str)
> >> --
> >> 2.7.3
> >>
> >> _______________________________________________
> >> mesa-dev mailing list
> >> mesa-dev at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160527/c9f3ea89/attachment-0001.html>