[Mesa-dev] [PATCH 7/7] intel/compiler: Release memory used by NIR metadata before generating machine code

[Jason Ekstrand]

On Mon, Nov 26, 2018 at 12:43 PM Ian Romanick <idr at freedesktop.org> wrote:

> On 11/21/2018 11:08 AM, Jason Ekstrand wrote:
> > On Tue, Nov 20, 2018 at 6:40 PM Ian Romanick <idr at freedesktop.org
> > <mailto:idr at freedesktop.org>> wrote:
> >
> >     From: Ian Romanick <ian.d.romanick at intel.com
> >     <mailto:ian.d.romanick at intel.com>>
> >
> >     Changes in peak memory usage according to Valgrind massif:
> >
> >     mean soft fp64 using uint64:   1,342,766,051 => 1,010,677,195
> >     gfxbench5 aztec ruins high 11:    62,369,974 =>    62,369,974
> >     deus ex mankind divided 148:      62,290,717 =>    62,290,717
> >     deus ex mankind divided 2890:     72,105,042 =>    71,930,254
> >     dirt showdown 676:                72,243,383 =>    72,243,383
> >     dolphin ubershaders 210:          80,650,936 =>    80,650,936
> >
> >     Signed-off-by: Ian Romanick <ian.d.romanick at intel.com
> >     <mailto:ian.d.romanick at intel.com>>
> >     ---
> >      src/intel/compiler/brw_fs.cpp   | 6 ++++++
> >      src/intel/compiler/brw_vec4.cpp | 6 ++++++
> >      2 files changed, 12 insertions(+)
> >
> >     diff --git a/src/intel/compiler/brw_fs.cpp
> >     b/src/intel/compiler/brw_fs.cpp
> >     index 3e083723471..e0e6b34d41a 100644
> >     --- a/src/intel/compiler/brw_fs.cpp
> >     +++ b/src/intel/compiler/brw_fs.cpp
> >     @@ -7170,6 +7170,12 @@ brw_compile_fs(const struct brw_compiler
> >     *compiler, void *log_data,
> >         prog_data->barycentric_interp_modes =
> >            brw_compute_barycentric_interp_modes(compiler->devinfo,
> shader);
> >
> >     +   /* Release memory used by NIR metadata.  It is not needed any
> >     longer. */
> >     +   nir_foreach_function(function, shader) {
> >     +      if (function->impl)
> >     +         nir_live_ssa_defs_impl_flush(function->impl);
> >     +   }
> >     +
> >         cfg_t *simd8_cfg = NULL, *simd16_cfg = NULL, *simd32_cfg = NULL;
> >
> >         fs_visitor v8(compiler, log_data, mem_ctx, key,
> >     diff --git a/src/intel/compiler/brw_vec4.cpp
> >     b/src/intel/compiler/brw_vec4.cpp
> >     index 74a4d09fc79..3401d999a15 100644
> >     --- a/src/intel/compiler/brw_vec4.cpp
> >     +++ b/src/intel/compiler/brw_vec4.cpp
> >     @@ -2947,6 +2947,12 @@ brw_compile_vs(const struct brw_compiler
> >     *compiler, void *log_data,
> >            brw_print_vue_map(stderr, &prog_data->base.vue_map);
> >         }
> >
> >     +   /* Release memory used by NIR metadata.  It is not needed any
> >     longer. */
> >     +   nir_foreach_function(function, shader) {
> >     +      if (function->impl)
> >     +         nir_live_ssa_defs_impl_flush(function->impl);
> >     +   }
> >
> >
> > I just had a look at this and it looks like nir_sweep will also clean up
> > the metadata.  Maybe we should just add a nir_sweep() call to the end of
> > brw_postprocess_nir()?  That would clean up other artifacts of
> > stage-specific lowering as well and putting it in brw_postprocess_nir()
> > would make it apply to all stages.
>
> It turns out that we do call nir_sweep at the end of
> brw_postprocess_nir.  I'm reluctant to add code to flush the metadata to
> nir_sweep for a couple reasons.
>

Except the last thing sweep_impl does is blow out the metadata...

Now I'm very confused.  We do call nir_sweep at the end of
brw_postprocess_nir and it doesn't sweep the metadata and it does declare
it as non-existing... So why isn't the memory getting freed?  There's
something we're missing that's causing the metadata to hang around when it
shouldn't.  Or maybe I'm just being extraordinarily thick.

--Jason


> nir_sweep is named for the sweep part of mark-and-sweep garbage
> collecting.  Currently, that is what it does.  Adding code to release
> memory that is still referenced (though not needed) is something different.
>
> nir_sweep is called in a handful of other places, and it's not 100%
> obvious to me that none of these places will want any of the metadata.
>
> >     +
> >         if (is_scalar) {
> >            prog_data->base.dispatch_mode = DISPATCH_MODE_SIMD8;
> >
> >     --
> >     2.14.4
> >
> >     _______________________________________________
> >     mesa-dev mailing list
> >     mesa-dev at lists.freedesktop.org <mailto:
> mesa-dev at lists.freedesktop.org>
> >     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181126/744793d7/attachment-0001.html>