On Wed, Mar 11, 2015 at 6:28 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote: > I think I mentioned this at least once before, but I don't think that this > is a good long-term solution. The nir_lower_io function does two things > primarily. 1) It assigns each variable a "device location" and 2) changes > the variable loads to loads with an offset. In the long-term, I think we > want to split 1) out and let the backend control what "device location" gets > assigned to each input/output. Then, add a knob to 2) to select how we want > things packed. Doing this will let us avoid this whole re-mapping mess and, > in the case of uniforms, better control things so that we can push some of > them. That said, this is *not* a NAK of the current patch. If you want to > hold off on that for a while, this looks ok. > --Jason
Yes, some re-thinking of how inputs/outputs/uniforms are handled would certainly be useful. For some history, when I wrote the old code (and Jason's code is essentially doing the same thing but in a different/better way) it was more of a "let's get this working for now" sort of thing since I wasn't sure what a cleaner solution would look like (I hadn't even written the backend code yet!). But I agree that scalar VS support shouldn't have to block on that rewrite. > > On Mon, Mar 9, 2015 at 1:58 AM, Kenneth Graunke <kenn...@whitecape.org> > wrote: >> >> Signed-off-by: Kenneth Graunke <kenn...@whitecape.org> >> --- >> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 23 ++++++++++++++++++++++- >> 1 file changed, 22 insertions(+), 1 deletion(-) >> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp >> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp >> index 3baafc4..1734d03 100644 >> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp >> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp >> @@ -199,11 +199,32 @@ fs_visitor::nir_setup_inputs(nir_shader *shader) >> struct hash_entry *entry; >> hash_table_foreach(shader->inputs, entry) { >> nir_variable *var = (nir_variable *) entry->data; >> + enum brw_reg_type type = brw_type_for_base_type(var->type); >> fs_reg input = offset(nir_inputs, var->data.driver_location); >> >> fs_reg reg; >> switch (stage) { >> - case MESA_SHADER_VERTEX: >> + case MESA_SHADER_VERTEX: { >> + /* Our ATTR file is indexed by VERT_ATTRIB_*, which is the value >> + * stored in nir_variable::location. >> + * >> + * However, NIR's load_input intrinsics use a different index - >> an >> + * offset into a single contiguous array containing all inputs. >> + * This index corresponds to the nir_variable::driver_location >> field. >> + * >> + * So, we need to copy from fs_reg(ATTR, var->location) to >> + * offset(nir_inputs, var->data.driver_location). >> + */ >> + unsigned components = var->type->without_array()->components(); >> + unsigned array_length = var->type->is_array() ? >> var->type->length : 1; >> + for (unsigned i = 0; i < array_length; i++) { >> + for (unsigned j = 0; j < components; j++) { >> + emit(MOV(retype(offset(input, components * i + j), type), >> + offset(fs_reg(ATTR, var->data.location + i, >> type), j))); >> + } >> + } >> + break; >> + } >> case MESA_SHADER_GEOMETRY: >> case MESA_SHADER_COMPUTE: >> unreachable("fs_visitor not used for these stages yet."); >> -- >> 2.2.1 >> >> _______________________________________________ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev