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.
On Jul 27, 2016 8:37 AM, "Emil Velikov" <emil.l.veli...@gmail.com> wrote:
> On 27 July 2016 at 16:17, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> > On Jul 27, 2016 6:03 AM, "Emil Velikov" <emil.l.veli...@gmail.com>
> wrote:
> >>
> >> From: Emil Velikov <emil.veli...@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-sta...@lists.freedesktop.org>
> >> Cc: Jason Ekstrand <ja...@jlekstrand.net>
> >> Signed-off-by: Emil Velikov <emil.veli...@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
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
