[Mesa-dev] [PATCH 1/6] anv: hook internal validate layer only for debug builds

[Jason Ekstrand]

On Jul 27, 2016 6:03 AM, "Emil Velikov" <emil.l.velikov at gmail.com> wrote:
>
> From: Emil Velikov <emil.velikov at collabora.com>
>
> Presently the layer has only a single entry point, which uses asserts
solely.
> Thus even on release builds the function (and thus the whole layer) will
do
> nothing but adding runtime and binary(size) overhead.
>
> Cc: "12.0" <mesa-stable at lists.freedesktop.org>
> Cc: Jason Ekstrand <jason at jlekstrand.net>
> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> ---
> We can use a ANV_USE_VALIDATE macro which atm will default to == DEBUG.
> ---
>  src/intel/vulkan/anv_entrypoints_gen.py | 36
+++++++++++++++++++++++----------
>  src/intel/vulkan/anv_image.c            |  2 ++
>  2 files changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_entrypoints_gen.py
b/src/intel/vulkan/anv_entrypoints_gen.py
> index 2896174..a4922c2 100644
> --- a/src/intel/vulkan/anv_entrypoints_gen.py
> +++ b/src/intel/vulkan/anv_entrypoints_gen.py
> @@ -134,7 +134,9 @@ if opt_header:
>          print "%s gen75_%s%s;" % (type, name, args)
>          print "%s gen8_%s%s;" % (type, name, args)
>          print "%s gen9_%s%s;" % (type, name, args)
> +        print "#ifdef DEBUG"
>          print "%s anv_validate_%s%s;" % (type, name, args)
> +        print "#endif // DEBUG"
>          print_guard_end(name)
>      exit()
>
> @@ -185,14 +187,7 @@ for type, name, args, num, h in entrypoints:
>      print "   \"vk%s\\0\"" % name
>      offsets.append(i)
>      i += 2 + len(name) + 1
> -print """   ;
> -
> -/* Weak aliases for all potential validate functions. These will resolve
to
> - * NULL if they're not defined, which lets the resolve_entrypoint()
function
> - * either pick a validate wrapper if available or just plug in the actual
> - * entry point.
> - */
> -"""
> +print "   ;"
>
>  # Now generate the table of all entry points and their validation
functions
>
> @@ -201,7 +196,18 @@ for type, name, args, num, h in entrypoints:
>      print "   { %5d, 0x%08x }," % (offsets[num], h)
>  print "};\n"
>
> +print """
> +
> +/* Weak aliases for all potential validate functions. These will resolve
to
> + * NULL if they're not defined, which lets the resolve_entrypoint()
function
> + * either pick a validate wrapper if available or just plug in the actual
> + * entry point.
> + */
> +"""
> +
>  for layer in [ "anv", "validate", "gen7", "gen75", "gen8", "gen9" ]:
> +    if "validate" in layer:
> +        print "#ifdef DEBUG"
>      for type, name, args, num, h in entrypoints:
>          print_guard_start(name)
>          print "%s %s_%s%s __attribute__ ((weak));" % (type, layer, name,
args)
> @@ -211,13 +217,13 @@ for layer in [ "anv", "validate", "gen7", "gen75",
"gen8", "gen9" ]:
>          print_guard_start(name)
>          print "   .%s = %s_%s," % (name, layer, name)
>          print_guard_end(name)
> +    if "validate" in layer:
> +        print "#endif // DEBUG"
>      print "};\n"
>
>  print """
>  #ifdef DEBUG
>  static bool enable_validate = true;
> -#else
> -static bool enable_validate = false;
>  #endif
>
>  /* We can't use symbols that need resolving (like, oh, getenv) in the
resolve
> @@ -231,8 +237,14 @@ determine_validate(void)
>  {
>     const char *s = getenv("ANV_VALIDATE");
>
> -   if (s)
> +   if (s) {
> +#ifdef DEBUG
>        enable_validate = atoi(s);
> +#else
> +      fprintf(stderr, "libvulkand-intel was built without VALIDATE
support.\\n");
> +      abort();
> +#endif
> +   }
>  }
>
>  static const struct brw_device_info *dispatch_devinfo;
> @@ -246,8 +258,10 @@ anv_set_dispatch_devinfo(const struct
brw_device_info *devinfo)
>  void * __attribute__ ((noinline))
>  anv_resolve_entrypoint(uint32_t index)
>  {
> +#ifdef DEBUG
>     if (enable_validate && validate_layer.entrypoints[index])
>        return validate_layer.entrypoints[index];
> +#endif
>
>     if (dispatch_devinfo == NULL) {
>        return anv_layer.entrypoints[index];
> diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
> index dff51bc..43d19f5 100644
> --- a/src/intel/vulkan/anv_image.c
> +++ b/src/intel/vulkan/anv_image.c
> @@ -332,6 +332,7 @@ void anv_GetImageSubresourceLayout(
>     }
>  }
>
> +#ifdef DEBUG
>  VkResult
>  anv_validate_CreateImageView(VkDevice _device,

As long as anv_entrypoints contains a declaration of this function, we
don't need to #ifdef around it.  Hopefully, the linker is smart enough to
detect that it's not used and delete the code.

Otherwise, this patch seems perfectly reasonable for now.  At one point we
had the idea that we could use the validate endpoints to do some extra
validation that would be shipped in release mode but disabled by default at
zero cost.  However, we're not really taking advantage of that now and do
we might as well drop it.

>                               const VkImageViewCreateInfo *pCreateInfo,
> @@ -406,6 +407,7 @@ anv_validate_CreateImageView(VkDevice _device,
>
>     return anv_CreateImageView(_device, pCreateInfo, pAllocator, pView);
>  }
> +#endif // DEBUG
>
>  static struct anv_state
>  alloc_surface_state(struct anv_device *device,
> --
> 2.9.0
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160727/46762d80/attachment-0001.html>