<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Apr 10, 2018 at 10:26 PM, Timothy Arceri <span dir="ltr"><<a href="mailto:tarceri@itsqueeze.com" target="_blank">tarceri@itsqueeze.com</a>></span> wrote:<span></span><br><span></span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span> <br> On 11/04/18 15:05, Jason Ekstrand wrote:<br> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> If I understand correctly, this is because when running with minimal GLSL IR, opt_function_inlining doesn't acutally inline them all. Is that correct? If so, would it make sense to just repeatedly call do_function_inlining until it stops making progress when we're running with minimal optimizations? Not that I'm opposed to using NIR for more things, but it bothers me a bit that reducing GLSL IR optimizations is causing us to break previous assumptions about what is, effectively, a lowering pass.<br> </blockquote> <br></span> The st currently calls do_common_optimization() in a loop to lower these away. Which even just calling one time adds something like 8% compile times to the Deus Ex shaders.<br></blockquote><div><br></div><div>Ouch.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> What are the assumptions you worried about breaking? There are currently bugs which are fixed by this series (see patch 5 which I think also needs to copy struct members and expression operands now that I think about it), I'm not confident at all that calling do_function_inlining() on its own would even work due to what is described in commit 6, I'm pretty sure there is some sketchy reliance on opt passes for things to work correctly during unrolling.<br></blockquote><div><br></div><div>Ugh... Yeah, that's sketchy. I'm mostly concerned about other hidden assumptions. For instance, we assume that all arrays have sizes. Then again, as you pointed out, we may not actually be guaranteed that those assumptions hold today. :-) If Jenkins is happy, I think I can handle my own nervousness. :-)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> I'd really like to avoid the unnecessary loops.<br></blockquote><div><br></div><div>Yeah, I totally get that.<br><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"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="m_-1790947414348950467h5"> <br> On Mon, Apr 9, 2018 at 9:34 PM, Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com" target="_blank">tarceri@itsqueeze.com</a> <mailto:<a href="mailto:tarceri@itsqueeze.com" target="_blank">tarceri@itsqueeze.com</a>><wbr>> wrote:<br> <br> ---<br> src/compiler/glsl/glsl_to_ni<wbr>r.cpp | 20 ++++++++++++++++++++<br> 1 file changed, 20 insertions(+)<br> <br> diff --git a/src/compiler/glsl/glsl_to_ni<wbr>r.cpp<br> b/src/compiler/glsl/glsl_to_ni<wbr>r.cpp<br> index 5a36963607e..55c01024669 100644<br> --- a/src/compiler/glsl/glsl_to_ni<wbr>r.cpp<br> +++ b/src/compiler/glsl/glsl_to_ni<wbr>r.cpp<br> @@ -26,6 +26,7 @@<br> */<br> <br> #include "glsl_to_nir.h"<br> +#include "ir_optimization.h"<br> #include "ir_visitor.h"<br> #include "ir_hierarchical_visitor.h"<br> #include "ir.h"<br> @@ -161,6 +162,25 @@ glsl_to_nir(const struct gl_shader_program<br> *shader_prog,<br> v2.run(sh->ir);<br> visit_exec_list(sh->ir, &v1);<br> <br> + nir_validate_shader(shader);<br> +<br> + /* We have to lower away local constant initializers right before we<br> + * inline functions. That way they get properly initialized at<br> the top<br> + * of the function and not at the top of its caller.<br> + */<br> + nir_lower_constant_initialize<wbr>rs(shader, nir_var_local);<br> + nir_lower_returns(shader);<br> + nir_inline_functions(shader);<br> +<br> + /* Now that we have inlined everything remove all of the<br> functions except<br> + * main().<br> + */<br> + foreach_list_typed_safe(nir_f<wbr>unction, function, node,<br> &(shader)->functions){<br> + if (strcmp("main", function->name) != 0) {<br> + exec_node_remove(&function->n<wbr>ode);<br> + }<br> + }<br> +<br> nir_lower_constant_initializer<wbr>s(shader, (nir_variable_mode)~0);<br> <br> /* Remap the locations to slots so those requiring two slots<br> will occupy<br> --<br> 2.17.0<br> <br> ______________________________<wbr>_________________<br> mesa-dev mailing list<br></div></div> <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.freedes<wbr>ktop.org</a>><br> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br> <<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org<wbr>/mailman/listinfo/mesa-dev</a>><br> <br> <br> </blockquote> </blockquote></div><br></div></div>