<html><head></head><body><div>On Mon, 2017-10-30 at 11:39 -0700, Jason Ekstrand wrote:</div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Oct 30, 2017 at 12:43 AM, Iago Toral <span dir="ltr"><<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>></span> wrote:<br><blockquote type="cite"><div><span class=""><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="m_-2464964315506121072HOEnZb"><div class="m_-2464964315506121072h5"></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></span><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></blockquote><div><br></div><div>Done.<br></div><div> </div><blockquote type="cite"><div><div>Also, I just noticed something wrong:</div><div><br></div><div>(...)</div><span class=""><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="m_-2464964315506121072HOEnZb"><div class="m_-2464964315506121072h5">
> diff --git a/src/intel/compiler/brw_fs.cp<wbr>p<br>
> b/src/intel/compiler/brw_fs.cp<wbr>p<br>
> index 52079d3..75139fd 100644<br>
> --- a/src/intel/compiler/brw_fs.cp<wbr>p<br>
> +++ b/src/intel/compiler/brw_fs.cp<wbr>p<br>
> @@ -1956,8 +1956,10 @@ void<br>
> fs_visitor::assign_constant_l<wbr>ocations()<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_lo<wbr>c);<br></div></div></blockquote></div></div></div></blockquote><div><br></div></span><div>It is impossible that the assert is ever hit. I'd suggest to drop it or maybe change it by:</div></div><br></blockquote><div><br></div><div>The if condition is for push and the assert is for pull.</div></div></div></div></blockquote><div><br></div><div>Wow! I read it twice because I thought you would be doing that and I read it wrong both times, sorry about that :-(</div><div><br></div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>--Jason<br></div><div> </div><blockquote type="cite"><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><span class=""><div><br></div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote type="cite"><div class="m_-2464964315506121072HOEnZb"><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_ni<wbr>r.cpp<br>
> b/src/intel/compiler/brw_fs_ni<wbr>r.cpp<br>
> index 7556576..05efee3 100644<br>
> --- a/src/intel/compiler/brw_fs_ni<wbr>r.cpp<br>
> +++ b/src/intel/compiler/brw_fs_ni<wbr>r.cpp<br>
> @@ -81,8 +81,11 @@ fs_visitor::nir_setup_outputs(<wbr>)<br>
> void<br>
> fs_visitor::nir_setup_uniform<wbr>s()<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_lo<wbr>c);</div></div></blockquote></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote type="cite"><div class="m_-2464964315506121072HOEnZb"><div class="m_-2464964315506121072h5">
> return;<br>
> + }<br>
> <br>
> uniforms = nir->num_uniforms / 4;<br>
> }<br>
</div></div><br></blockquote></div><br></div></div>
</blockquote></span></div><br></blockquote></div><br></div></div>
</blockquote></body></html>