On Sat, Nov 14, 2015 at 1:00 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote: > On Sat, Nov 14, 2015 at 9:44 AM, Rob Clark <robdcl...@gmail.com> wrote: >> On Sat, Nov 14, 2015 at 12:30 PM, Jason Ekstrand <ja...@jlekstrand.net> >> wrote: >>> On Sat, Nov 14, 2015 at 8:58 AM, Rob Clark <robdcl...@gmail.com> wrote: >>>> On Sat, Nov 14, 2015 at 11:01 AM, Jason Ekstrand <ja...@jlekstrand.net> >>>> wrote: >>>>> On Thu, Nov 12, 2015 at 7:30 AM, Iago Toral <ito...@igalia.com> wrote: >>>>>> On Thu, 2015-11-12 at 16:23 +0100, Iago Toral wrote: >>>>>>> Patches 1-4 are, >>>>>>> Reviewed-by: Iago Toral Quiroga <ito...@igalia.com> >>>>>>> >>>>>>> Patch 5 seems to be missing. >>>>> >>>>> If it helps to calm reviewer's minds, I ran patches 1-5 with this patch >>>>> on top: >>>>> >>>>> http://cgit.freedesktop.org/~jekstrand/mesa/commit/?h=wip/nir-clone >>>>> >>>>> Zero regressions in piglit, dEQP, and the CTS. >>>> >>>> imho, please push something like this to master, w/ perhaps an env-var >>>> switch (ofc just for debug builds).. this way we can work nir_clone >>>> testing into normal CI test cycle, and protect against future >>>> difficult-to-track-down breakage >>> >>> I thought about doing that but it didn't really work very well with >>> patch 6. Also, by the time we get to patch 7, it's getting tested >>> pretty well. About the only thing that doesn't get tested there is >>> registers. I'm not opposed to adding support for testing it in CI, >>> but I don't want to dirty up an API to do so if it can be avoided. >>> Would you be ok with cloning in a few key places? >> >> Well, I prefer testing between each stage.. it's a little brute-force, >> but it ensures we don't miss something that only appears between >> certain stages, now or in the future. The few-key-places approach is >> certainly better than nothing. >> >> I guess the 'dirty up an API' bit referred to returning nir_shader's? >> I don't think that is *that* horrible a price to pay.. > > I'm more concerned about the fact that you get out a pointer that may > or may not be the one you passed in and we may or may not have deleted > the one you passed in and, to make things better, if you accidentally > ignore the return value it will work fine unless INTEL_NIR_CLONE is > enabled. I think we're going to find ourselves breaking the nir_clone > testing code more often than breaking nir_clone.
hmm, I don't think it is *that* hard, plus if NIR_TEST_CLONE is enabled on a regular bases in CI tests, it shouldn't be very hard to keep it all working properly. (we should put the pass-runner macros plus env-var for running clone test somewhere in nir, rather than i965, so all the drivers are following the same pattern, but that is a different topic) BR, -R > --Jason > >> BR, >> -R >> >>> --Jason >>> >>>> (and you can even pre-emptively slap my r-b on that, since I'm happy >>>> however that is accomplished..) >>>> >>>> BR, >>>> -R >>>> >>>>>> Oh never mind, I've just seen your reply to the thread pointing to the >>>>>> repository. >>>>>> >>>>>> Iago >>>>>> >>>>>>> Iago >>>>>>> >>>>>>> On Wed, 2015-11-11 at 17:23 -0800, Jason Ekstrand wrote: >>>>>>> > On older hardware (Iron Lake and below), we can't support texture >>>>>>> > rectangle >>>>>>> > natively. Sandy Bridge through Haswell can support it but don't >>>>>>> > support >>>>>>> > the GL_CLAMP wrap mode natively. It isn't until Broadwell that >>>>>>> > GL_CLAMP is >>>>>>> > supported together with GL_TEXTURE_RECTANGLE in hardware. In the >>>>>>> > cases >>>>>>> > where it isn't supported, we have to fake it by dividing by the >>>>>>> > texture >>>>>>> > size. >>>>>>> > >>>>>>> > Previously, we had a rescale_texcoord function added a uniform to >>>>>>> > hold the >>>>>>> > texture coordinate and used that to rescale/clamp the texture >>>>>>> > coordinates. >>>>>>> > For a while now, nir_lower_tex has been able to lower texture >>>>>>> > rectangle to >>>>>>> > a textureSize and a regular texture2D operation. This series makes >>>>>>> > i965 >>>>>>> > use the nir_lower_tex path instead. Incidentally, this fixes texture >>>>>>> > rectangle support in vertex and geometry shaders on Haswell and below. >>>>>>> > (The backend lowering was only ever done in the FS backend.) >>>>>>> > >>>>>>> > Since this is the first time we're doing any sort of shader variants >>>>>>> > in >>>>>>> > NIR, the first several passes add the infastructure to do so. Two of >>>>>>> > these >>>>>>> > patches are from Ken, two are from Rob, and one (nir_clone itself) is >>>>>>> > my >>>>>>> > rendition but heavily based on what Rob did only with less hashing. >>>>>>> > >>>>>>> > Jason Ekstrand (7): >>>>>>> > nir: support to clone shaders >>>>>>> > i965/nir: Split shader optimization and lowering into three satages >>>>>>> > i965: Move postprocess_nir to codegen time >>>>>>> > nir/lower_tex: Report progress >>>>>>> > nir/lower_tex: Set the dest_type for txs instructions >>>>>>> > i965/fs: Don't allow SINT32 as a return type for resinfo >>>>>>> > i965: Use nir_lower_tex for texture coordinate lowering >>>>>>> > >>>>>>> > Kenneth Graunke (2): >>>>>>> > i965/nir: Add OPT() and OPT_V() macros for invoking NIR passes. >>>>>>> > i965/nir: Validate that NIR passes call nir_metadata_preserve(). >>>>>>> > >>>>>>> > Rob Clark (2): >>>>>>> > nir: remove nir_variable::max_ifc_array_access >>>>>>> > nir: add array length field >>>>>>> > >>>>>>> > src/glsl/Makefile.sources | 1 + >>>>>>> > src/glsl/nir/glsl_to_nir.cpp | 14 +- >>>>>>> > src/glsl/nir/nir.c | 8 + >>>>>>> > src/glsl/nir/nir.h | 27 +- >>>>>>> > src/glsl/nir/nir_clone.c | 671 >>>>>>> > ++++++++++++++++++++++ >>>>>>> > src/glsl/nir/nir_lower_tex.c | 20 +- >>>>>>> > src/glsl/nir/nir_metadata.c | 36 ++ >>>>>>> > src/mesa/drivers/dri/i965/brw_fs.cpp | 13 +- >>>>>>> > src/mesa/drivers/dri/i965/brw_fs.h | 3 - >>>>>>> > src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 10 +- >>>>>>> > src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 4 +- >>>>>>> > src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 125 ---- >>>>>>> > src/mesa/drivers/dri/i965/brw_nir.c | 268 +++++---- >>>>>>> > src/mesa/drivers/dri/i965/brw_nir.h | 15 + >>>>>>> > src/mesa/drivers/dri/i965/brw_vec4.cpp | 7 +- >>>>>>> > src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp | 8 +- >>>>>>> > 16 files changed, 966 insertions(+), 264 deletions(-) >>>>>>> > create mode 100644 src/glsl/nir/nir_clone.c >>>>>>> > >>>>>>> >>>>>> >>>>>> >>>>> _______________________________________________ >>>>> 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