[Mesa-dev] [PATCH 00/12] anv: Add support for the variablePointers feature

[Jason Ekstrand]

On Thu, Nov 30, 2017 at 11:52 AM, Kristian Høgsberg <hoegsberg at gmail.com>
wrote:

> On Thu, Oct 19, 2017 at 11:04 AM, Jason Ekstrand <jason at jlekstrand.net>
> wrote:
> > Not to be confused with variablePointersStorageBuffer which is the
> > subset of VK_KHR_variable_pointers required to enable the extension.
> > This gives us "full" support for variable pointers.
> >
> > The approach chosen here was to do the lowering to _shared intrinsics
> > directly in spirv_to_nir instead of using the _var intrinsics and using
> > nir_lower_io.  Pointers with a storage class of Workgroup are given an
> > implicit std430 layout and now go through the same offset pointer paths
> as
> > UBO and SSBO access.  The whole thing really ended up working out rather
> > cleanly.
> >
> > There are some downsides to this approach.  One, is that we can't delete
> > unused shared variables post-optimization.  Also, the driver may be able
> to
> > handle better than std430.  Both of these can lead to some waisted SLM
> > space.  This also means that we can't do any deref-based load/store
> > elimination optimizations on SLM but we didn't really before so that's no
> > great loss; SLM is now exactly as hard to optimize as SSBOs.
>
> I was going to take a quick look at this and ended up doing a proper
> review. Very readable and clean series. I thought it was a little odd
> (unmotivated) to add the struct vtn_builder argument to
> vtn_pointer_uses_ssa_offset in "spirv: Refactor a couple of pointer
> query helpers"  when it isn't used until "spirv: Add support for
> lowering workgroup access to offsets", but whatevs.


I mostly did that because I wanted to avoid churn in the patch that started
us using it.  It's annoying to have to touch all those places again.


> I'm curious how
> much better you think a backend should be able to pack? What kind of
> improvements over std430 can you realistically expect?


We would probably do a bit better because we'd just tightly pack everything
with maybe do natural alignment but it's probably not a huge improvement.


> Also,what kind
> of unused shared variables wouldn't nir itself be able to remove?
>

Once we've lowered to offsets in SPIR-V -> NIR, everything has a fixed
address/offset and there's no good way to shuffle things around and
re-pack.  If the shader has a bunch of variables which aren't statically
used (only used inside an "if (0)" for instance) then we would be able to
eliminate them during the NIR optimization process and reduce the size of
SLM.  My primary fear was when processing a module with a large number of
shaders that we would cause the shared memory space to explode with
variables that aren't used but patch 2 should get rid of most of that.


> Reviewed-by: Kristian H. Kristensen <hoegsberg at google.com>
>

Thanks!


>
> > Connor, Yes, I know that this is not quite the approach you were
> suggesting
> > on IRC.  I considered how we might add some sort of deref intrinsic and I
> > don't see a good way of doing so without rewriting large chunks of NIR.
> I
> > think that rewrite is probably worth it some day but that day is not
> today.
> > We people asking for this feature so I really don't want to delay on a
> > major NIR rewrite.
> >
> > Cc: Connor Abbott <cwabbott0 at gmail.com>
> > Cc: Chad Versace <chadversary at chromium.org>
> > Cc: Dave Airlie <airlied at redhat.com>
> >
> > Jason Ekstrand (12):
> >   spirv: Drop the impl field from vtn_builder
> >   spirv: Only emit functions which are actually used
> >   spirv: Use a dereference instead of vtn_variable_resource_index
> >   spirv: Add a switch statement for the block store opcode
> >   spirv: Refactor the base case of offset_pointer_dereference
> >   spirv: Convert the supported_extensions struct to spirv_options
> >   spirv: Refactor a couple of pointer query helpers
> >   spirv: Use offset_pointer_dereference to instead of
> >     get_vulkan_resource_index
> >   spirv: Add theoretical support for single component pointers
> >   spirv: Rename get_shared_nir_atomic_op to get_var_nir_atomic_op
> >   spirv: Add support for lowering workgroup access to offsets
> >   anv: Add support for the variablePointers feature
> >
> >  src/amd/vulkan/radv_shader.c       |  23 ++--
> >  src/compiler/spirv/nir_spirv.h     |  34 ++++--
> >  src/compiler/spirv/spirv_to_nir.c  | 180 ++++++++++++++++++++++++-----
> >  src/compiler/spirv/vtn_cfg.c       |   4 +-
> >  src/compiler/spirv/vtn_private.h   |  30 +++--
> >  src/compiler/spirv/vtn_variables.c | 229 ++++++++++++++++++++++++------
> -------
> >  src/intel/vulkan/anv_device.c      |   2 +-
> >  src/intel/vulkan/anv_pipeline.c    |  25 ++--
> >  8 files changed, 372 insertions(+), 155 deletions(-)
> >
> > --
> > 2.5.0.400.gff86faf
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171130/88e9138e/attachment.html>