[Mesa-dev] [PATCH 1/5] nir: Define image load, store and atomic intrinsics.

[Francisco Jerez]

Connor Abbott <cwabbott0 at gmail.com> writes:

> On IRC, Ken and I were discussing using a scheme inspired by SPIR-V,
> which has an OpImagePointer instruction that forms a pointer to the
> particular texel of the image as well as
> OpAtomic{Load,Store,Exchange,etc.} that operate on an image or shared
> buffer pointer. The advantages would be:
>
> * Makes translating from SPIR-V easier.
> * Reduces the number of intrinsics we need to add for SSBO support.
> * Reduces the combinatorial explosion enough that we can have separate
> versions for 2, 3, and 4 components and MS vs. non-MS without it being
> unbearable. I'm not sure how much of a benefit that would be though.
>
> The disadvantages I can think of are:
>
> * Doesn't actually save any code in the i965 backend, since we need to
> do different things depending on if the pointer is to an image or a
> shared buffer anyways.
> * We'd have to special case nir_convert_from_ssa to ignore the SSA
> value that's really a pointer since we don't have any real type-level
> support for pointers.
> * Since we lower to SSA before converting to i965, there are some ugly
> edge cases when the coordinate argument becomes part of a phi web and
> gets potentially overwritten before the instruction that uses the
> pointer.
>
Yeah, I actually played around with a SPIR-V-like approach, and decided
to give up the idea in the end mainly because of the disadvantages you
mention, because it's nothing close to what the back-ends will want.

Two other ideas occurred to me that could have made the back-end's life
easier, but it didn't seem like they were worth doing:

 - Write a driver-independent lowering pass to convert SPIR-V-style
   intrinsics into coordinate-based intrinsics while still in SSA form.
   In that case there's likely no point in having the SPIR-V-style
   intrinsics in the first place, no back-end I know of will prefer
   image intrinsics in that form at this point, and we can just let the
   SPIR-V front-end deal with this problem alone.

 - Actually agree on a representation for a texel pointer.  A texel
   pointer would be a triple of pointer-to-object, vec4 of coordinates
   and sample index, together with some static metadata determining the
   memory access properties.  Something more back-end-specific would
   likely work too.  In any case we would also have to agree on a
   representation for a pointer to an image/SSB object.  And we would
   need to type-check pointers correctly to prevent the optimizer from
   doing illegal transformations (e.g. a single store intrinsic that
   writes to either coherent or non-coherent memory depending on the
   parent block, or, if we adhere to the limitations of GLSL strictly, a
   single store intrinsic that might access images based on different
   array uniforms).

   It gets even more "interesting" if you consider the limitations some
   back-ends have about accessing images non-uniformly -- We would have
   to guarantee that the pointer-to-object inside the texel pointer has
   the same value for all thread invocations executing a given load,
   store or atomic intrinsic, what implies that we would have to forbid
   texel pointers in phi instructions unless it can be proven that
   either the control flow incident into the node is uniform *or* the
   pointer-to-object coming from all parent branches is the same.

Sounds like a lot of work for little benefit at this point.

> I don't have a preference one way or the other, and I guess we could
> always refactor it later if we wanted to, so assuming Ken is OK with
> this, then besides one minor comment on patch 4 the series is
>
> Reviewed-by: Connor Abbott <cwabbott0 at gmail.com>
>
Thanks.

> On Tue, May 5, 2015 at 4:29 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>> ---
>>  src/glsl/nir/nir_intrinsics.h | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/src/glsl/nir/nir_intrinsics.h b/src/glsl/nir/nir_intrinsics.h
>> index 8e28765..4b13c75 100644
>> --- a/src/glsl/nir/nir_intrinsics.h
>> +++ b/src/glsl/nir/nir_intrinsics.h
>> @@ -89,6 +89,33 @@ ATOMIC(inc, 0)
>>  ATOMIC(dec, 0)
>>  ATOMIC(read, NIR_INTRINSIC_CAN_ELIMINATE)
>>
>> +/*
>> + * Image load, store and atomic intrinsics.
>> + *
>> + * All image intrinsics take an image target passed as a nir_variable.  Image
>> + * variables contain a number of memory and layout qualifiers that influence
>> + * the semantics of the intrinsic.
>> + *
>> + * All image intrinsics take a four-coordinate vector and a sample index as
>> + * first two sources, determining the location within the image that will be
>> + * accessed by the intrinsic.  Components not applicable to the image target
>> + * in use are equal to zero by convention.  Image store takes an additional
>> + * four-component argument with the value to be written, and image atomic
>> + * operations take either one or two additional scalar arguments with the same
>> + * meaning as in the ARB_shader_image_load_store specification.
>> + */
>> +INTRINSIC(image_load, 2, ARR(4, 1), true, 4, 1, 0,
>> +          NIR_INTRINSIC_CAN_ELIMINATE)
>> +INTRINSIC(image_store, 3, ARR(4, 1, 4), false, 0, 1, 0, 0)
>> +INTRINSIC(image_atomic_add, 3, ARR(4, 1, 1), true, 1, 1, 0, 0)
>> +INTRINSIC(image_atomic_min, 3, ARR(4, 1, 1), true, 1, 1, 0, 0)
>> +INTRINSIC(image_atomic_max, 3, ARR(4, 1, 1), true, 1, 1, 0, 0)
>> +INTRINSIC(image_atomic_and, 3, ARR(4, 1, 1), true, 1, 1, 0, 0)
>> +INTRINSIC(image_atomic_or, 3, ARR(4, 1, 1), true, 1, 1, 0, 0)
>> +INTRINSIC(image_atomic_xor, 3, ARR(4, 1, 1), true, 1, 1, 0, 0)
>> +INTRINSIC(image_atomic_exchange, 3, ARR(4, 1, 1), true, 1, 1, 0, 0)
>> +INTRINSIC(image_atomic_comp_swap, 4, ARR(4, 1, 1, 1), true, 1, 1, 0, 0)
>> +
>>  #define SYSTEM_VALUE(name, components) \
>>     INTRINSIC(load_##name, 0, ARR(), true, components, 0, 0, \
>>     NIR_INTRINSIC_CAN_ELIMINATE | NIR_INTRINSIC_CAN_REORDER)
>> --
>> 2.3.5
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150508/23ae6e5e/attachment.sig>