[Mesa-dev] [PATCH 2/2] glsl: Only call do_common_optimization once

[Thomas Helland]

Forwarding to the mailing list because I'm an idiot and am apparently not
able to email.

> 2016-11-13 10:39 GMT+01:00 Timothy Arceri <timothy.arceri at collabora.com>:
> > On Sun, 2016-11-13 at 02:29 +0100, Thomas Helland wrote:
> >> It turns out that only 8% of shaders benefit from more than one run
> >> of the do_common_optimization function, now that it has the loop.
> >> Doing only one run makes no difference to instruction count with NIR.
> >>
> >> 8324 shaders were run
> >
> > This is a small amount of shaders I take it this is just the public
> > shader-db?
> >
>
> It's an old one I've been running on for ages, so it's not very complete.
> Some old dota and tf2 shaders, plus some other games.
> I was actually planning on scraping a reasonable collection of games
> for shaders this evening, as I have the time.
>
> > While the series is interesting I think there are more robust ways to
> > deal with the issue of slow compile times. Landing the shader-cache
> > (once I rebase it on the recent changes I've made) is obviously one but
> > we are also getting very close to being able to use the NIR
> > optimisation passes from the GLSL IR linker (at least for things that
> > have a NIR backend). Once we do that we can work towards only calling
> > the GLSL IR passes from the compiler.
> >
> > Since I landed changes to switch i965 to using nir_gather_shader_info()
> > this week the only big blocker for using NIR opts in the linker is a
> > varying packing pass for NIR, once we have that is should be fairly
> > easy to wire things up like so in brw_create_nir() for i965:
> >
> >    nir = brw_preprocess_nir(brw->screen->compiler, nir);
> >
> >    //TODO: remove from GLSL IR the varyings/uniform that the nir opt
> >    // passes removed. This is safe because at this point all we want
> >    // from the GLSL IR is to assign locations and do validation on
> >    // varyings and uniforms we don't care if we make the IR invalid.
> >
> >    //TODO: call GLSL assign varying/uniform location/validation passes
> >
> >    //TODO: now remove from nir any more varyings that GLSL IR removed
> >    // (this is safe because these will just be varyings that are only
> >    //  used in one of the stages) and add locations that were assigned
> >    // in GLSL to NIR.
> >
> >    //TODO: free GLSL IR
> >
> >    //TODO: add a NIR varying packing pass here
> >
> > We also need theses patches I'm still waiting on review for [1][2].
> >
> > Anyway the point is IMO your time would probably be better spent
> > working towards improving deficiencies in the NIR opts so we can switch
> > to only using them faster as opposed to spending to much time improving
> > the GLSL IR opts.
> >
> >
> > [1] https://patchwork.freedesktop.org/patch/112044/
> > [2] https://patchwork.freedesktop.org/patch/112045/
> >
>
> Yes, I definitely agree that this is just a band aid, and doesn't fix
> the real issue.
> It was merely a simple test to check what effects this could probably
have,
> while being not that invasive of a change.
>
> >
> >> 8128 benefit from 1 calls to do_common_optimization
> >>  679 benefit from 2 calls to do_common_optimization
> >>   69 benefit from 3 calls to do_common_optimization
> >> ---
> >>  src/compiler/glsl/glsl_parser_extras.cpp | 5 ++---
> >>  1 file changed, 2 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp
> >> b/src/compiler/glsl/glsl_parser_extras.cpp
> >> index a5a0837..a1a1f90 100644
> >> --- a/src/compiler/glsl/glsl_parser_extras.cpp
> >> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
> >> @@ -1944,9 +1944,8 @@ _mesa_glsl_compile_shader(struct gl_context
> >> *ctx, struct gl_shader *shader,
> >>        /* Do some optimization at compile time to reduce shader IR
> >> size
> >>         * and reduce later work if the same shader is linked multiple
> >> times
> >>         */
> >> -      while (do_common_optimization(shader->ir, false, false,
> >> options,
> >> -                                    ctx->Const.NativeIntegers))
> >> -         ;
> >> +      do_common_optimization(shader->ir, false, false, options,
> >> +                                    ctx->Const.NativeIntegers);
> >>
> >>        validate_ir_tree(shader->ir);
> >>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161114/186982c3/attachment.html>