[Mesa-dev] [PATCH 09/21] nir/lower_io: Separate driver_location and base offset for uniforms

[Kenneth Graunke]

On Monday, August 24, 2015 04:51:48 PM Kenneth Graunke wrote:
> On Monday, August 24, 2015 04:14:13 PM Jason Ekstrand wrote:
> > On Mon, Aug 24, 2015 at 4:03 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> > > On Wednesday, August 19, 2015 10:45:44 PM Jason Ekstrand wrote:
> > >> v2 (Jason Ekstrand): Fix up image uniforms
> > >> ---
> > >>  src/glsl/nir/nir_lower_io.c              |  9 +++++++--
> > >>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 13 +------------
> > >>  2 files changed, 8 insertions(+), 14 deletions(-)
> > >>
> > >> diff --git a/src/glsl/nir/nir_lower_io.c b/src/glsl/nir/nir_lower_io.c
> > >> index 15a4edc..70645b6 100644
> > >> --- a/src/glsl/nir/nir_lower_io.c
> > >> +++ b/src/glsl/nir/nir_lower_io.c
> > >> @@ -244,9 +244,14 @@ nir_lower_io_block(nir_block *block, void *void_state)
> > >>           nir_src indirect;
> > >>           unsigned offset = get_io_offset(intrin->variables[0],
> > >>                                           &intrin->instr, &indirect, state);
> > >> -         offset += intrin->variables[0]->var->data.driver_location;
> > >>
> > >> -         load->const_index[0] = offset;
> > >> +         unsigned location = intrin->variables[0]->var->data.driver_location;
> > >> +         if (mode == nir_var_uniform) {
> > >> +            load->const_index[0] = location;
> > >> +            load->const_index[1] = offset;
> > >> +         } else {
> > >> +            load->const_index[0] = location + offset;
> > >> +         }
> > >>
> > >>           if (has_indirect)
> > >>              load->src[0] = indirect;
> > >> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > >> index 5b54b95..32a05dc 100644
> > >> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > >> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> > >> @@ -239,18 +239,7 @@ fs_visitor::nir_setup_uniform(nir_variable *var)
> > >>        }
> > >>
> > >>        if (storage->type->is_image()) {
> > >> -         /* Images don't get a valid location assigned by nir_lower_io()
> > >> -          * because their size is driver-specific, so we need to allocate
> > >> -          * space for them here at the end of the parameter array.
> > >> -          */
> > >> -         var->data.driver_location = uniforms;
> > >> -         unsigned size =
> > >> -            BRW_IMAGE_PARAM_SIZE * MAX2(storage->array_elements, 1);
> > >> -
> > >> -         setup_image_uniform_values(uniforms, storage);
> > >> -
> > >> -         param_size[uniforms] = size;
> > >> -         uniforms += size;
> > >> +         setup_image_uniform_values(index, storage);
> > >>        } else {
> > >>           unsigned slots = storage->type->component_slots();
> > >>           if (storage->array_elements)
> > >>
> > >
> > > This hunk really looks like it was squashed into the wrong patch...
> > 
> > You're right.  This hunk should go into the patch that starts passing
> > type_size into nir_lower_io.
> 
> Ahh, that makes so much more sense.  Squashing this hunk into that
> patch (07) sounds great.  Perhaps add something like this to the commit
> message of that patch:
> 
> "In fact, this causes nir_lower_io to allocate space for our image
>  uniforms, allowing us to drop hacks in the backend."
> 
> Patches 1-13 (other than the ones I wrote) are:
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
> 
> This definitely cleans up a lot of mess.  Great work!

Erp.  Except for patch 06.  I don't like that one, and proposed an
alternate solution.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150824/5cfc272a/attachment.sig>