[Mesa-dev] [PATCH 1/2] i965: retype result of typed atomic operation to the same type as source

[Ilia Mirkin]

On Mon, Feb 22, 2016 at 5:38 PM, Francisco Jerez <currojerez at riseup.net> wrote:
> Ilia Mirkin <imirkin at alum.mit.edu> writes:
>
>> On Mon, Feb 22, 2016 at 5:21 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>>> Ilia Mirkin <imirkin at alum.mit.edu> writes:
>>>
>>>> On Mon, Feb 22, 2016 at 3:53 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>> On Mon, Feb 22, 2016 at 3:52 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>>> This fixes atomic exchange with a r32f image, as needed by
>>>>>> GL_OES_shader_atomic_exchange.
>>>>>
>>>>> Sorry, that should be GL_OES_shader_image_atomic of course.
>>>>>
>>>>>>
>>>>>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>>>>> ---
>>>>>>  src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp | 6 ++++--
>>>>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp b/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp
>>>>>> index 081dbad..e775cc0 100644
>>>>>> --- a/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp
>>>>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp
>>>>>> @@ -156,8 +156,10 @@ namespace brw {
>>>>>>           const fs_reg tmp = bld.vgrf(BRW_REGISTER_TYPE_UD, n);
>>>>>>           bld.LOAD_PAYLOAD(tmp, srcs, n, 0);
>>>>>>
>>>>>> -         return emit_send(bld, SHADER_OPCODE_TYPED_ATOMIC_LOGICAL,
>>>>>> -                          addr, tmp, surface, dims, op, rsize);
>>>>>> +         return retype(
>>>>>> +            emit_send(bld, SHADER_OPCODE_TYPED_ATOMIC_LOGICAL,
>>>>>> +                      addr, tmp, surface, dims, op, rsize),
>>>>>> +            src0.type);
>>>>
>>>> Actually I've been thinking a bit more about this... relying on
>>>> src0.type won't fly. The surface's mesa_format needs to be passed
>>>> through to emit_image_atomic, just like it is done for emit_image_load
>>>> and emit_image_store. And I need to fix up the vec4 emitter. Curro,
>>>> does that sound reasonable? Or should the retype be done at the
>>>> emit_image_atomic callsites?
>>>>
>>> I doubt it's necessary to pass the surface format through.
>>
>> Well, you have to get the format from somewhere. You could easily have e.g.
>>
>> int foo, bar;
>>
>> foo = floatBitsAsInt(imageAtomicExchange(img, intBitsAsFloat(bar)));
>>
>> I believe in this scenario, src0.type is likely to be "int" instead of
>> "float". Both of our fixes suffer from this problem.
>
> No, the src type is only dependent on the image type.  If the image is
> float you'll get a float.

Ah yes, quite right:

      const fs_reg src0 = (info->num_srcs >= 3 ?
                           retype(get_nir_src(instr->src[2]), base_type) :
                           fs_reg());

[where base_type is the image's type]

>
>>
>>> Any reason
>>> we shouldn't go with this series I sent half a year ago instead? (and you
>>> actually commented to)
>>>
>>> https://lists.freedesktop.org/archives/mesa-dev/2015-August/091948.html
>>> https://lists.freedesktop.org/archives/mesa-dev/2015-August/091949.html
>>> https://lists.freedesktop.org/archives/mesa-dev/2015-August/091950.html
>>> https://lists.freedesktop.org/archives/mesa-dev/2015-August/091951.html
>>> https://lists.freedesktop.org/archives/mesa-dev/2015-August/091952.html
>>
>> Mainly that I forgot all about it :) Not sure if I commented on it
>> back then, but I don't think we need a separate enable bit. Definitely
>> not for TGSI (which is a lot more typeless), and it seems like not for
>> i965 either. Also note that this behaviour is core in GLES 3.2 (and GL
>> 4.5).
>>
> IIRC the series I sent already enabled the built-ins for GLES3.2 and
> GL4.5 even though GLES3.2 had just been released.  I don't think any
> additional work is required.

Probably. Either way it needs rebasing/etc... perhaps worth a resend?

  -ilia