<html><head></head><body><div>On Fri, 2017-10-27 at 12:43 -0700, Jason Ekstrand wrote:</div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Oct 27, 2017 at 12:35 AM, Iago Toral <span dir="ltr"><<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>></span> wrote:<br><blockquote type="cite">This sounds good to me, but I guess it is not really fixing anything,<br>
right? I ask because the subject claims that this patch does something<br>
that the original code was already supposed to be doing.<br><div class="HOEnZb"><div class="h5"></div></div><br></blockquote><div><br></div><div>This patch is a bit of an artifact of history.  I needed it at one point in the development of the series but I think it may have ended up not mattering in the end.  I still think it's a nice clean-up.</div></div></div></div></blockquote><div><br></div><div>Ok, fair enough. In that case I'd suggest to change the subject to something like this:</div><div><br></div><div>"intel/fs: use pull constant locations to check for first compile of a shader"</div><div><br></div><div>Also, I just noticed something wrong:</div><div><br></div><div>(...)</div><div><br></div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><blockquote type="cite"><div class="HOEnZb"><div class="h5">
> diff --git a/src/intel/compiler/brw_fs.<wbr>cpp<br>
> b/src/intel/compiler/brw_fs.<wbr>cpp<br>
> index 52079d3..75139fd 100644<br>
> --- a/src/intel/compiler/brw_fs.<wbr>cpp<br>
> +++ b/src/intel/compiler/brw_fs.<wbr>cpp<br>
> @@ -1956,8 +1956,10 @@ void<br>
>  fs_visitor::assign_constant_<wbr>locations()<br>
>  {<br>
>     /* Only the first compile gets to decide on locations. */<br>
> -   if (dispatch_width != min_dispatch_width)<br>
> +   if (push_constant_loc) {<br>
> +      assert(pull_constant_<wbr>loc);<br></div></div></blockquote></div></div></div></blockquote><div><br></div><div>It is impossible that the assert is ever hit. I'd suggest to drop it or maybe change it by:</div><div><br></div><div>assert(dispatch_width != min_dispatch_width);</div><div><br></div><div>But since you get rid of min_dispatch_with in the next patch, maybe just drop the assert.</div><div><br></div><div>The same in nir_setup_uniforms below.</div><div><br></div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote type="cite"><div class="HOEnZb"><div>
>        return;<br>
> +   }<br>
>  <br>
>     bool is_live[uniforms];<br>
>     memset(is_live, 0, sizeof(is_live));<br>
> diff --git a/src/intel/compiler/brw_fs_<wbr>nir.cpp<br>
> b/src/intel/compiler/brw_fs_<wbr>nir.cpp<br>
> index 7556576..05efee3 100644<br>
> --- a/src/intel/compiler/brw_fs_<wbr>nir.cpp<br>
> +++ b/src/intel/compiler/brw_fs_<wbr>nir.cpp<br>
> @@ -81,8 +81,11 @@ fs_visitor::nir_setup_outputs(<wbr>)<br>
>  void<br>
>  fs_visitor::nir_setup_<wbr>uniforms()<br>
>  {<br>
> -   if (dispatch_width != min_dispatch_width)<br>
> +   /* Only the first compile gets to set up uniforms. */<br>
> +   if (push_constant_loc) {<br>
> +      assert(pull_constant_<wbr>loc);</div></div></blockquote></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote type="cite"><div class="HOEnZb"><div class="h5">
>        return;<br>
> +   }<br>
>  <br>
>     uniforms = nir->num_uniforms / 4;<br>
>  }<br>
</div></div><br></blockquote></div><br></div></div>
</blockquote></body></html>