I looked at the one validate function we still have and I don't think it validates anything that won't trigger asserts elsewhere. Let's just kill it all. <div class="gmail_extra"> <div class="gmail_quote">On Jul 27, 2016 8:37 AM, "Emil Velikov" <<a href="mailto:emil.l.velikov@gmail.com">emil.l.velikov@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 27 July 2016 at 16:17, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote: > On Jul 27, 2016 6:03 AM, "Emil Velikov" <<a href="mailto:emil.l.velikov@gmail.com">emil.l.velikov@gmail.com</a>> wrote: >> >> From: Emil Velikov <<a href="mailto:emil.velikov@collabora.com">emil.velikov@collabora.com</a>> >> >> 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" <<a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.org</a>> >> Cc: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> >> Signed-off-by: Emil Velikov <<a href="mailto:emil.velikov@collabora.com">emil.velikov@collabora.com</a>> >> --- >> 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 </blockquote></div></div>