[Mesa-dev] [PATCH v3 23/48] intel/fs: Assign constant locations if they haven't been assigned

[Iago Toral]

On Mon, 2017-10-30 at 11:39 -0700, Jason Ekstrand wrote:
> On Mon, Oct 30, 2017 at 12:43 AM, Iago Toral <itoral at igalia.com>
> wrote:
> > On Fri, 2017-10-27 at 12:43 -0700, Jason Ekstrand wrote:
> > > On Fri, Oct 27, 2017 at 12:35 AM, Iago Toral <itoral at igalia.com>
> > > wrote:
> > > > This sounds good to me, but I guess it is not really fixing
> > > > anything,
> > > > 
> > > > right? I ask because the subject claims that this patch does
> > > > something
> > > > 
> > > > that the original code was already supposed to be doing.
> > > > 
> > > 
> > > 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.
> > 
> > Ok, fair enough. In that case I'd suggest to change the subject to
> > something like this:
> > 
> > "intel/fs: use pull constant locations to check for first compile
> > of a shader"
> 
> Done.
>  
> > Also, I just noticed something wrong:
> > 
> > (...)
> > 
> > > > > diff --git a/src/intel/compiler/brw_fs.cpp
> > > > 
> > > > > b/src/intel/compiler/brw_fs.cpp
> > > > 
> > > > > index 52079d3..75139fd 100644
> > > > 
> > > > > --- a/src/intel/compiler/brw_fs.cpp
> > > > 
> > > > > +++ b/src/intel/compiler/brw_fs.cpp
> > > > 
> > > > > @@ -1956,8 +1956,10 @@ void
> > > > 
> > > > >  fs_visitor::assign_constant_locations()
> > > > 
> > > > >  {
> > > > 
> > > > >     /* Only the first compile gets to decide on locations. */
> > > > 
> > > > > -   if (dispatch_width != min_dispatch_width)
> > > > 
> > > > > +   if (push_constant_loc) {
> > > > 
> > > > > +      assert(pull_constant_loc);
> > 
> > It is impossible that the assert is ever hit. I'd suggest to drop
> > it or maybe change it by:
> 
> The if condition is for push and the assert is for pull.

Wow! I read it twice because I thought you would be doing that and I
read it wrong both times, sorry about that  :-(
> --Jason
>  
> > assert(dispatch_width != min_dispatch_width);
> > 
> > But since you get rid of min_dispatch_with in the next patch, maybe
> > just drop the assert.
> > 
> > The same in nir_setup_uniforms below.
> > 
> > > > >        return;
> > > > 
> > > > > +   }
> > > > 
> > > > >  
> > > > 
> > > > >     bool is_live[uniforms];
> > > > 
> > > > >     memset(is_live, 0, sizeof(is_live));
> > > > 
> > > > > diff --git a/src/intel/compiler/brw_fs_nir.cpp
> > > > 
> > > > > b/src/intel/compiler/brw_fs_nir.cpp
> > > > 
> > > > > index 7556576..05efee3 100644
> > > > 
> > > > > --- a/src/intel/compiler/brw_fs_nir.cpp
> > > > 
> > > > > +++ b/src/intel/compiler/brw_fs_nir.cpp
> > > > 
> > > > > @@ -81,8 +81,11 @@ fs_visitor::nir_setup_outputs()
> > > > 
> > > > >  void
> > > > 
> > > > >  fs_visitor::nir_setup_uniforms()
> > > > 
> > > > >  {
> > > > 
> > > > > -   if (dispatch_width != min_dispatch_width)
> > > > 
> > > > > +   /* Only the first compile gets to set up uniforms. */
> > > > 
> > > > > +   if (push_constant_loc) {
> > > > 
> > > > > +      assert(pull_constant_loc);
> > > > >        return;
> > > > 
> > > > > +   }
> > > > 
> > > > >  
> > > > 
> > > > >     uniforms = nir->num_uniforms / 4;
> > > > 
> > > > >  }
> > > > 
> > > > 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171031/0c628e37/attachment.html>