[Mesa-dev] [PATCH 8/7] i965: Accurately bail on SIMD16 compiles.

[Ilia Mirkin]

On Wed, Mar 12, 2014 at 4:28 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> Ideally, we'd like to never even attempt the SIMD16 compile if we could
> know ahead of time that it won't succeed---it's purely a waste of time.
> This is especially important for state-based recompiles, which happen at
> draw time.
>
> The fragment shader compiler has a number of checks like:
>
>    if (dispatch_width == 16)
>       fail("...some reason...");
>
> This patch introduces a new no16() function which replaces the above
> pattern.  In the SIMD8 compile, it sets a "SIMD16 will never work" flag.
> Then, brw_wm_fs_emit can check that flag, skip the SIMD16 compile, and
> issue a helpful performance warning if INTEL_DEBUG=perf is set.  (In
> SIMD16 mode, no16() calls fail(), for safety's sake.)
>
> The great part is that this is not a heuristic---if the flag is set, we
> know with 100% certainty that the SIMD16 compile would fail.  (It might
> fail anyway if we run out of registers, but it's always worth trying.)
>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_fs.cpp         | 69 +++++++++++++++++++++++-----
>  src/mesa/drivers/dri/i965/brw_fs.h           |  4 ++
>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 44 +++++++++---------
>  3 files changed, 83 insertions(+), 34 deletions(-)
>
> I forgot to send this one out...it applies on top of the previous 7 patches.
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 62848be..9ad80c5 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -647,9 +647,8 @@ fs_visitor::emit_shader_time_write(enum shader_time_shader_type type,
>  }
>
>  void
> -fs_visitor::fail(const char *format, ...)
> +fs_visitor::vfail(const char *format, va_list va)
>  {
> -   va_list va;
>     char *msg;
>
>     if (failed)
> @@ -657,9 +656,7 @@ fs_visitor::fail(const char *format, ...)
>
>     failed = true;
>
> -   va_start(va, format);
>     msg = ralloc_vasprintf(mem_ctx, format, va);
> -   va_end(va);
>     msg = ralloc_asprintf(mem_ctx, "FS compile failed: %s\n", msg);
>
>     this->fail_msg = msg;
> @@ -669,6 +666,49 @@ fs_visitor::fail(const char *format, ...)
>     }
>  }
>
> +void
> +fs_visitor::fail(const char *format, ...)
> +{
> +   va_list va;
> +
> +   va_start(va, format);
> +   vfail(format, va);
> +   va_end(va);
> +}
> +
> +/**
> + * Mark this program as impossible to compile in SIMD16 mode.
> + *
> + * During the SIMD8 compile (which happens first), we can detect and flag
> + * things that are unsupported in SIMD16 mode, so the compiler can skip
> + * the SIMD16 compile altogether.
> + *
> + * During a SIMD16 compile (if one happens anyway), this just calls fail().
> + */
> +void
> +fs_visitor::no16(const char *format, ...)
> +{
> +   va_list va;
> +
> +   va_start(va, format);
> +
> +   if (dispatch_width == 16) {
> +      vfail(format, va);

I think there's a va_end() missing in this path. Not sure what the
end-effect of that is, but I'm pretty sure that the recommendation is
to always end the list before returning.

> +      return;
> +   }
> +
> +   simd16_unsupported = true;
> +
> +   if (INTEL_DEBUG & DEBUG_PERF) {
> +      if (no16_msg)
> +         ralloc_vasprintf_append(&no16_msg, format, va);
> +      else
> +         no16_msg = ralloc_vasprintf(mem_ctx, format, va);
> +   }
> +
> +   va_end(va);
> +}
> +