[Mesa-dev] [PATCH 00/11] i965/nir: Do texture rectangle lowering in NIR
Rob Clark
robdclark at gmail.com
Mon Nov 16 07:15:40 PST 2015
On Sat, Nov 14, 2015 at 1:00 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Sat, Nov 14, 2015 at 9:44 AM, Rob Clark <robdclark at gmail.com> wrote:
>> On Sat, Nov 14, 2015 at 12:30 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>>> On Sat, Nov 14, 2015 at 8:58 AM, Rob Clark <robdclark at gmail.com> wrote:
>>>> On Sat, Nov 14, 2015 at 11:01 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>>>>> On Thu, Nov 12, 2015 at 7:30 AM, Iago Toral <itoral at igalia.com> wrote:
>>>>>> On Thu, 2015-11-12 at 16:23 +0100, Iago Toral wrote:
>>>>>>> Patches 1-4 are,
>>>>>>> Reviewed-by: Iago Toral Quiroga <itoral at 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 at lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list