I wasn't happy with the redundancy either but at the time just didn't see
how to refactor it more cleanly.  I'll take a second pass to consolidate
and remove duplication.

- Chuck

On Tue, Jan 16, 2018 at 5:03 PM, Cherniak, Bruce <[email protected]>
wrote:

>
> > On Jan 16, 2018, at 1:59 PM, Chuck Atkins <[email protected]>
> wrote:
> >
> > Part 2 of 2 (part 1 is autoconf changes, part 2 is C++ changes)
> >
> > When only a single SWR architecture is being used, this allows that
> > architecture to be builtin rather than as a separate libswrARCH.so that
> > gets loaded via dlopen.  Since there are now several different code
> > paths for each detected CPU architecture, the log output is also
> > adjusted to convey where the backend is getting loaded from.
> >
> > This allows SWR to be used for static mesa builds which are still
> > important for large HPC environments where shared libraries can impose
> > unacceptable application startup times as hundreds of thousands of copies
> > of the libs are loaded from a shared parallel filesystem.
> >
> > Based on an initial implementation by Tim Rowley.
> >
> > Signed-off-by: Chuck Atkins <[email protected]>
> > CC: Tim Rowley <[email protected]>
> > CC: Bruce Cherniak <[email protected]>
> > ---
> > src/gallium/drivers/swr/swr_loader.cpp | 94
> ++++++++++++++++++++++------------
> > 1 file changed, 60 insertions(+), 34 deletions(-)
> >
> > diff --git a/src/gallium/drivers/swr/swr_loader.cpp
> b/src/gallium/drivers/swr/swr_loader.cpp
> > index 9d6f918e34..57f826fd59 100644
> > --- a/src/gallium/drivers/swr/swr_loader.cpp
> > +++ b/src/gallium/drivers/swr/swr_loader.cpp
> > @@ -31,78 +31,104 @@
> > struct pipe_screen *
> > swr_create_screen(struct sw_winsys *winsys)
> > {
> > +   struct pipe_screen *screen = swr_create_screen_internal(winsys);
> > +
> > +   bool found = false;
> > +   bool is_knl = false;
> > +
> > +#ifndef HAVE_SWR_BUILTIN
> >    char filename[256] = { 0 };
> > -   fprintf(stderr, "SWR detected ");
> > +#endif
> >
> > -   util_dl_library *pLibrary = nullptr;
> >
> >    util_cpu_detect();
> >
> > -   bool is_knl = false;
> > -
> > -   if (!strlen(filename) &&
> > -       util_cpu_caps.has_avx512f && util_cpu_caps.has_avx512er) {
> > -#if HAVE_SWR_KNL
> > -      fprintf(stderr, "KNL ");
> > +   if (!found && util_cpu_caps.has_avx512f &&
> util_cpu_caps.has_avx512er) {
> > +      fprintf(stderr, "SWR detected KNL instruction support ");
> > +#ifndef HAVE_SWR_KNL
> > +      fprintf(stderr, "(skipping; not built).\n");
> > +#else
>
> Each of the below #ifdef HAVE_SWR_BUILTIN clauses are identical, except
> the filename.
>
> Could you instead just setup the filename, set found = true (and the one
> is_knl), then do
> the BUILTIN work all in one place...
>
> > +   #ifdef HAVE_SWR_BUILTIN
> > +      swr_screen(screen)->pfnSwrGetInterface = SwrGetInterface;
> > +      fprintf(stderr, "(using; builtin).\n");
> > +   #else
> >       sprintf(filename, "%s%s%s", UTIL_DL_PREFIX, "swrKNL", UTIL_DL_EXT);
> > +      fprintf(stderr, "(using; %s).\n", filename);
> > +   #endif
>
> > +      found = true;
> >       is_knl = true;
> > -#else
> > -      fprintf(stderr, "KNL (not built) ");
> > #endif
> >    }
> >
> > -   if (!strlen(filename) &&
> > -       util_cpu_caps.has_avx512f && util_cpu_caps.has_avx512bw) {
> > -#if HAVE_SWR_SKX
> > -      fprintf(stderr, "SKX ");
> > -      sprintf(filename, "%s%s%s", UTIL_DL_PREFIX, "swrSKX",
> UTIL_DL_EXT);
> > +   if (!found && util_cpu_caps.has_avx512f &&
> util_cpu_caps.has_avx512bw) {
> > +      fprintf(stderr, "SWR detected SKX instruction support ");
> > +#ifndef HAVE_SWR_SKX
> > +      fprintf(stderr, "(skipping; not built).\n");
> > #else
> > -      fprintf(stderr, "SKX (not built) ");
>
> > +   #ifdef HAVE_SWR_BUILTIN
> > +      swr_screen(screen)->pfnSwrGetInterface = SwrGetInterface;
> > +      fprintf(stderr, "(using; builtin).\n");
> > +   #else
> > +      sprintf(filename, "%s%s%s", UTIL_DL_PREFIX, "swrSKX",
> UTIL_DL_EXT);
> > +      fprintf(stderr, "(using; %s).\n", filename);
> > +   #endif
>
> > +      found = true;
> > #endif
> >    }
> >
> > -   if (!strlen(filename) && util_cpu_caps.has_avx2) {
> > -#if HAVE_SWR_AVX2
> > -      fprintf(stderr, "AVX2 ");
> > -      sprintf(filename, "%s%s%s", UTIL_DL_PREFIX, "swrAVX2",
> UTIL_DL_EXT);
> > +   if (!found && util_cpu_caps.has_avx2) {
> > +      fprintf(stderr, "SWR detected AVX2 instruction support ");
> > +#ifndef HAVE_SWR_AVX2
> > +      fprintf(stderr, "(skipping; not built).\n");
> > #else
> > -      fprintf(stderr, "AVX2 (not built) ");
>
> > +   #ifdef HAVE_SWR_BUILTIN
> > +      swr_screen(screen)->pfnSwrGetInterface = SwrGetInterface;
> > +      fprintf(stderr, "(using; builtin).\n");
> > +   #else
> > +      sprintf(filename, "%s%s%s", UTIL_DL_PREFIX, "swrAVX2",
> UTIL_DL_EXT);
> > +      fprintf(stderr, "(using; %s).\n", filename);
> > +   #endif
>
> > +      found = true;
> > #endif
> >    }
> >
> > -   if (!strlen(filename) && util_cpu_caps.has_avx) {
> > -#if HAVE_SWR_AVX
> > -      fprintf(stderr, "AVX ");
> > -      sprintf(filename, "%s%s%s", UTIL_DL_PREFIX, "swrAVX",
> UTIL_DL_EXT);
> > +   if (!found && util_cpu_caps.has_avx) {
> > +      fprintf(stderr, "SWR detected AVX instruction support ");
> > +#ifndef HAVE_SWR_AVX
> > +      fprintf(stderr, "(skipping; not built).\n");
> > #else
> > -      fprintf(stderr, "AVX (not built) ");
>
> > +   #ifdef HAVE_SWR_BUILTIN
> > +      swr_screen(screen)->pfnSwrGetInterface = SwrGetInterface;
> > +      fprintf(stderr, "(using; builtin).\n");
> > +   #else
> > +      sprintf(filename, "%s%s%s", UTIL_DL_PREFIX, "swrAVX",
> UTIL_DL_EXT);
> > +      fprintf(stderr, "(using; %s).\n", filename);
> > +   #endif
>
> > +      found = true;
> > #endif
> >    }
> >
> > -   if (!strlen(filename)) {
> > -      fprintf(stderr, "- no appropriate swr architecture library.
> Aborting!\n");
> > +   if (!found) {
> > +      fprintf(stderr, "SWR could not detect a supported CPU
> architecture.\n");
> >       exit(-1);
> > -   } else {
> > -      fprintf(stderr, "\n");
> >    }
> >
> > -   pLibrary = util_dl_open(filename);
> > -
>
> Here, you can do the BUILTIN
>       swr_screen(screen)->pfnSwrGetInterface = SwrGetInterface;
>       fprintf(stderr, "(using; builtin).\n");
>
> or open the correct library and print which one we're using (
> #else
> > +#ifndef HAVE_SWR_BUILTIN
> > +   util_dl_library *pLibrary = util_dl_open(filename);
> >    if (!pLibrary) {
> >       fprintf(stderr, "SWR library load failure: %s\n", util_dl_error());
> >       exit(-1);
> >    }
> >
> >    util_dl_proc pApiProc = util_dl_get_proc_address(pLibrary,
> "SwrGetInterface");
> > -
> >    if (!pApiProc) {
> >       fprintf(stderr, "SWR library search failure: %s\n",
> util_dl_error());
> >       exit(-1);
> >    }
> >
> > -   struct pipe_screen *screen = swr_create_screen_internal(winsys);
> >    swr_screen(screen)->pfnSwrGetInterface =
> (PFNSwrGetInterface)pApiProc;
> >    swr_screen(screen)->is_knl = is_knl;
> > +#endif
> >
> >    return screen;
> > }
> > --
> > 2.14.3
> >
>
>
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to