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

[Kristian Høgsberg]

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'm curious how
much better you think a backend should be able to pack? What kind of
improvements over std430 can you realistically expect? Also,what kind
of unused shared variables wouldn't nir itself be able to remove?

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


> 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