<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Nov 26, 2018 at 12:43 PM Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 11/21/2018 11:08 AM, Jason Ekstrand wrote:<br>
> On Tue, Nov 20, 2018 at 6:40 PM Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a><br>
> <mailto:<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>>> wrote:<br>
> <br>
>     From: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a><br>
>     <mailto:<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a>>><br>
> <br>
>     Changes in peak memory usage according to Valgrind massif:<br>
> <br>
>     mean soft fp64 using uint64:   1,342,766,051 => 1,010,677,195<br>
>     gfxbench5 aztec ruins high 11:    62,369,974 =>    62,369,974<br>
>     deus ex mankind divided 148:      62,290,717 =>    62,290,717<br>
>     deus ex mankind divided 2890:     72,105,042 =>    71,930,254<br>
>     dirt showdown 676:                72,243,383 =>    72,243,383<br>
>     dolphin ubershaders 210:          80,650,936 =>    80,650,936<br>
> <br>
>     Signed-off-by: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a><br>
>     <mailto:<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a>>><br>
>     ---<br>
>      src/intel/compiler/brw_fs.cpp   | 6 ++++++<br>
>      src/intel/compiler/brw_vec4.cpp | 6 ++++++<br>
>      2 files changed, 12 insertions(+)<br>
> <br>
>     diff --git a/src/intel/compiler/brw_fs.cpp<br>
>     b/src/intel/compiler/brw_fs.cpp<br>
>     index 3e083723471..e0e6b34d41a 100644<br>
>     --- a/src/intel/compiler/brw_fs.cpp<br>
>     +++ b/src/intel/compiler/brw_fs.cpp<br>
>     @@ -7170,6 +7170,12 @@ brw_compile_fs(const struct brw_compiler<br>
>     *compiler, void *log_data,<br>
>         prog_data->barycentric_interp_modes =<br>
>            brw_compute_barycentric_interp_modes(compiler->devinfo, shader);<br>
> <br>
>     +   /* Release memory used by NIR metadata.  It is not needed any<br>
>     longer. */<br>
>     +   nir_foreach_function(function, shader) {<br>
>     +      if (function->impl)<br>
>     +         nir_live_ssa_defs_impl_flush(function->impl);<br>
>     +   }<br>
>     +<br>
>         cfg_t *simd8_cfg = NULL, *simd16_cfg = NULL, *simd32_cfg = NULL;<br>
> <br>
>         fs_visitor v8(compiler, log_data, mem_ctx, key,<br>
>     diff --git a/src/intel/compiler/brw_vec4.cpp<br>
>     b/src/intel/compiler/brw_vec4.cpp<br>
>     index 74a4d09fc79..3401d999a15 100644<br>
>     --- a/src/intel/compiler/brw_vec4.cpp<br>
>     +++ b/src/intel/compiler/brw_vec4.cpp<br>
>     @@ -2947,6 +2947,12 @@ brw_compile_vs(const struct brw_compiler<br>
>     *compiler, void *log_data,<br>
>            brw_print_vue_map(stderr, &prog_data->base.vue_map);<br>
>         }<br>
> <br>
>     +   /* Release memory used by NIR metadata.  It is not needed any<br>
>     longer. */<br>
>     +   nir_foreach_function(function, shader) {<br>
>     +      if (function->impl)<br>
>     +         nir_live_ssa_defs_impl_flush(function->impl);<br>
>     +   }<br>
> <br>
> <br>
> I just had a look at this and it looks like nir_sweep will also clean up<br>
> the metadata.  Maybe we should just add a nir_sweep() call to the end of<br>
> brw_postprocess_nir()?  That would clean up other artifacts of<br>
> stage-specific lowering as well and putting it in brw_postprocess_nir()<br>
> would make it apply to all stages.<br>
<br>
It turns out that we do call nir_sweep at the end of<br>
brw_postprocess_nir.  I'm reluctant to add code to flush the metadata to<br>
nir_sweep for a couple reasons.<br></blockquote><div><br></div><div>Except the last thing sweep_impl does is blow out the metadata...</div><div><br></div><div>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.</div><div><br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
nir_sweep is named for the sweep part of mark-and-sweep garbage<br>
collecting.  Currently, that is what it does.  Adding code to release<br>
memory that is still referenced (though not needed) is something different.<br>
<br>
nir_sweep is called in a handful of other places, and it's not 100%<br>
obvious to me that none of these places will want any of the metadata.<br>
<br>
>     +<br>
>         if (is_scalar) {<br>
>            prog_data->base.dispatch_mode = DISPATCH_MODE_SIMD8;<br>
> <br>
>     -- <br>
>     2.14.4<br>
> <br>
>     _______________________________________________<br>
>     mesa-dev mailing list<br>
>     <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <mailto:<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a>><br>
>     <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
> <br>
<br>
</blockquote></div></div>