[Mesa-dev] [PATCH v2 00/16] ARB_gl_spirv series 3 v2: support for atomic counters

Tue Jul 3 12:41:12 UTC 2018

On 03/07/18 02:29, Timothy Arceri wrote:
> Series looks good to me. 

Just pushed to master. Thanks!

> How much to go before we can turn this extension on?

TL; DR: In summary there is a lot already done, but there is still work
to do. In general, we tried to cover as much features as possible, and
we are now dealing with the corner cases/specifics of each feature.

I give a more detailed, but somewhat quick/messy answer below. If you
want a more structured/detailed answer I could it write tomorrow and
send to the list.

As a reference, and after getting the atomic counters series pushed, our
development branch has 66 patches. That includes:

* XFB/Geometry streams: as with atomic counters, those features are not
supported on Vulkan, so most of this patches are on the spirv_to_nir
part. Right now this is the main candidate for the next series to be
send to review.

* ubo/ssbo support: a good bunch of those 66 patches are here. Most
common cases are supported. But in addition to some heavy cleaning (that
probably will lead to less patches due squashing), it needs more work on
the layouts support. Specifically, we are still dealing with std140 and
std430 layouts. But, the final solution would lead to support
general-defined layouts, as SPIR-V allows that. That would mean that we
would need to include on the glsl type the ArrayStride/MatrixStride/etc
coming from SPIR-V, and use that info instead of rely on
std140/std430/etc Right now we are debating internally if it would make
sense to go one step forward and propose to extend this to GLSL too.
Under that idea, GLSL_INTERFACE_PACKING_STD140/STD430 would disappear,
and the info would be included as strides/offsets on the type. Having
said so, we didn't explore that idea too much yet, as perhaps it is not
practical so big change on the GLSL linker at this moment. So perhaps we
would start doing that only on the ARB_gl_spirv linker first.

* Some misc patches

* ARB_spirv_extensions support: we sent that series some time ago too,
but then we decided to focus first on ARB_gl_spirv. That is enough for
the basic functionality of this extension. And obviously, it is not
needed to enable ARB_gl_spirv first. But it is a small extension, and I
think that could be enabled at the same time.

And those are the features that are implemented, there are also some
wip/missing minor subfeatures:

* AOA of ubos/ssbos: this was somewhat postponed until it was clarified
if it was needed or not. This was not supported on Vulkan, so we created
a spec issue to clarify if it was needed for ARB_gl_spirv or not.
Khronos concluded that it was needed. Most of the pending work is on the
spirv to nir part again, although somewhat more complex that with atomic
counters/XFB. Most of the pending test crashes come from here. In any
case, we could send in advance ubo/ssbo support patches, and send aoa
support later.

* Validations: most of the failures and crashes that are not aoa of ubos
come from here. Although we already have some patches that add some
cross-stage validation, in general ARB_gl_spirv validation rules are
fuzzy/non-specified. Although the spec explicitly says that intra-stage
validation is not needed, things are not so clear with cross-stage
validation. In theory linking/compilation should(could?) raise the same
errors that with GLSL. But names are optional on ARB_gl_spirv, and
linking should be based on location/binding/whatever. And names are the
base of most validation rules on GLSL. So in the end the validations
would need some "porting". It is so fuzzy that perhaps it is reasonable
to enable the extension without it. Now and then I wonder if it makes
sense to open a spec issue so this point gets clarified.

* Update program interface queries: as names can be absent by spec, and
are absent in the practice with the current implementation, all those
queries need some update, to at least avoid crashes if used. This was
postponed because in general, as the names are debug info, program
interface queries seems less reliable/useful in general (imho).

* GLSL cache integration: right now we only have some patches to avoid
loading the cache if we are using spirv shaders, in order to prevent
false positives and crashes. In the short term, before enabling the
extension, we need to do a more detailed work here. In the long term,
probably it would be good to try to get the cache working with SPIR-V
shaders too, so extending GLSL cache to Shader cache. But the latter can
be done after enabling the extension.

* Extra testing: we are using as much as possible tests from other
specs, but it would be good to do some testing with real applications
(if available)

And probably Im missing something, but I think that this is enough to
get a general idea.

FWIW, the pass rate with our custom piglit branch, that includes reuse
as much of possible tests from other specs:
* Current master: [34453/34453] skip: 5877, pass: 4519, fail: 2944,
crash: 21113
* Our development branch: [34453/34453] skip: 5877, pass: 28353, fail:
210, crash: 13

BR

>
> On 03/07/18 00:58, Alejandro Piñeiro wrote:
>> Hi Timothy. Thanks for the quick review!
>>
>> As you suggested some squash and commit drops, Im resending the v2 of
>> the series, just in case you want a final overview of the series
>> (although it is somewhat an overkill, I know).
>>
>> The only patch missing a review is "[PATCH 13/16] nir: Fix
>> OpAtomicCounterIDecrement for uniform atomic counters"
>>
>> As you suggested, the patch that added a utility to get the depth of
>> multidimensional arrays was dropped. So "[PATCH 07/16] nir/spirv: Fix
>> atomic counter (multidimensional-)arrays" is the patch that had more
>> changes, although those were the changes you suggested.
>>
>> The patches can be found here (rebased against today master).
>> https://github.com/Igalia/mesa/tree/arb_gl_spirv-series3-atomics-counters-v2
>>
>>
>> Again, thanks for the quick review
>>
>> Alejandro Piñeiro (10):
>>    compiler/glsl: refactor empty_uniform_block utilities to linker_util
>>    nir/linker: handle uniforms without explicit location
>>    spirv/nir: SpvStorageClassAtomicCounter support on
>>      vtn_storage_class_to_mode
>>    spirv/nir: add offset at vtn_variable
>>    nir_types: add glsl_atomic_uint_type() helper
>>    spirv/nir: tweak nir type when storage class is
>>      SpvStorageClassAtomicCounter
>>    spirv/nir: initialize offset on the nir var at vtn_create_variable
>>    spirv/nir: add atomic counter support on
>>      vtn_handle_ssbo_or_shared_atomic
>>    spirv/nir: add capability check for SpvCapabilityAtomicStorage
>>    i965: enable AtomicStorage capability for gen7+
>>
>> Antia Puentes (3):
>>    nir/spirv: Fix atomic counter (multidimensional-)arrays
>>    nir: Fix OpAtomicCounterIDecrement for uniform atomic counters
>>    mesa/glspirv: lower workgroup access to offsets
>>
>> Neil Roberts (3):
>>    nir/types: Add wrappers for a couple of atomic counter methods
>>    nir/linker: Add a pure NIR implementation of the atomic counter
>> linker
>>    i965: Use the new nir atomic counter linker for SPIR-V shaders
>>
>>   src/compiler/Makefile.sources                |   1 +
>>   src/compiler/glsl/gl_nir_link_atomics.c      | 282
>> +++++++++++++++++++++++++++
>>   src/compiler/glsl/gl_nir_link_uniforms.c     |  64 +++++-
>>   src/compiler/glsl/gl_nir_linker.h            |   3 +
>>   src/compiler/glsl/gl_nir_lower_atomics.c     |   8 +-
>>   src/compiler/glsl/glsl_to_nir.cpp            |   4 +-
>>   src/compiler/glsl/link_uniforms.cpp          |  34 +---
>>   src/compiler/glsl/linker.cpp                 |  19 +-
>>   src/compiler/glsl/linker.h                   |  13 --
>>   src/compiler/glsl/linker_util.cpp            |  55 ++++++
>>   src/compiler/glsl/linker_util.h              |  21 ++
>>   src/compiler/glsl/meson.build                |   1 +
>>   src/compiler/nir/nir_intrinsics.py           |   3 +-
>>   src/compiler/nir/nir_lower_atomics_to_ssbo.c |   8 +-
>>   src/compiler/nir_types.cpp                   |  18 ++
>>   src/compiler/nir_types.h                     |   4 +
>>   src/compiler/shader_info.h                   |   1 +
>>   src/compiler/spirv/spirv_to_nir.c            |  95 ++++++++-
>>   src/compiler/spirv/vtn_private.h             |   1 +
>>   src/compiler/spirv/vtn_variables.c           |  42 +++-
>>   src/mesa/drivers/dri/i965/brw_context.c      |   1 +
>>   src/mesa/drivers/dri/i965/brw_link.cpp       |   2 +
>>   src/mesa/main/glspirv.c                      |   1 +
>>   23 files changed, 599 insertions(+), 82 deletions(-)
>>   create mode 100644 src/compiler/glsl/gl_nir_link_atomics.c
>>
>