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

[Emil Velikov]

On 27 July 2016 at 16:17, Jason Ekstrand <jason at jlekstrand.net> wrote:
> 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.
>
So your suggestion is to drop the idef DEBUG hunk around the function
declaration(s) and this one ? Sure that will work.

The linker will only discard this if using the garbage collector
(already in place with $(GC_SECTIONS)) and/or LTO.

> 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.
>
Or if you want we can drop the validate code all together.

Up-to you really.
Emil