[Mesa-dev] [PATCH] glsl: trap atomic operations on illegal image formats in compiler.

[Dave Airlie]

On 6 May 2016 at 11:41, Francisco Jerez <currojerez at riseup.net> wrote:
> Dave Airlie <airlied at gmail.com> writes:
>
>> From: Dave Airlie <airlied at redhat.com>
>>
>> This fixes:
>> GL43-CTS.shader_image_load_store.negative-compileErrors
>>
>> where shader 9 was being compiled with atomic operation on an r16i.
>>
>> Signed-off-by: Dave Airlie <airlied at redhat.com>
>> ---
>>  src/compiler/glsl/ast_function.cpp      | 21 +++++++++++++++++++++
>>  src/compiler/glsl/builtin_functions.cpp |  6 +++---
>>  src/compiler/glsl/glsl_parser_extras.h  |  7 +++++++
>>  src/compiler/glsl/ir.h                  |  2 ++
>>  4 files changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/compiler/glsl/ast_function.cpp b/src/compiler/glsl/ast_function.cpp
>> index fe0df15..3ea6e05 100644
>> --- a/src/compiler/glsl/ast_function.cpp
>> +++ b/src/compiler/glsl/ast_function.cpp
>> @@ -139,6 +139,27 @@ verify_image_parameter(YYLTYPE *loc, _mesa_glsl_parse_state *state,
>>        return false;
>>     }
>>
>> +   if (formal->data.image_atomic) {
>> +     if (actual->data.image_format != GL_R32UI &&
>> +      actual->data.image_format != GL_R32I) {
>> +       _mesa_glsl_error(loc, state,
>> +                     "atomic operations can only happen on r32ui/r32i formats.");
>> +       return false;
>> +     }
>> +   }
>> +
>> +   if (formal->data.image_atomic_exchange) {
>> +     if ((actual->data.image_format != GL_R32UI &&
>> +       actual->data.image_format != GL_R32I)) {
>> +       /* check for the r32f special case. */
>> +       if (!(state->has_shader_image_atomic_exchange_float() &&
>> +          actual->data.image_format == GL_R32F)) {
>
> AFAICT the state->has_shader_image_atomic_exchange_float() check (and
> related changes below, including the definition of
> ir_variable_data::image_atomic_exchange) shouldn't be necessary because
> the floating point imageAtomicExchange overload shouldn't be introduced
> into the program symbol table if the r32f variant is not supported, so
> passing an r32f image would lead to a type mismatch either way
> regardless of what you do here.
>

Cool I can try that.

This bit of spec seems clear to me:

>From GLSL 4.50: Section 8.12

Atomic memory operations are supported on only a subset of all image
variable types; image must be
either:
• a signed integer image variable (type starts “iimage”) and a format
qualifier of r32i, used with a
data argument of type int, or
• an unsigned image variable (type starts “uimage”) and a format
qualifier of r32ui, used with a
data argument of type uint.

So there is another CTS test that does:

GLSL source for fragment shader 3:
: #version 420 core
layout(location = 0) out vec4 o_color;
layout(rgba32f) coherent volatile restrict uniform image2D g_image_layer0;
layout(rgba32f) volatile uniform image2D g_image_layer1;
vec4 Load(layout(rgba32f) volatile image2D image, ivec2 coord) {
  return imageLoad(image, coord);
}
void main() {
  ivec2 coord = ivec2(gl_FragCoord.xy);
  imageStore(g_image_layer0, coord, vec4(1.0));
  memoryBarrier();
  imageStore(g_image_layer0, coord, vec4(2.0));
  memoryBarrier();
  imageStore(g_image_layer0, coord, vec4(0.0, 1.0, 0.0, 1.0));
  memoryBarrier();
  o_color = imageLoad(g_image_layer0, coord) + Load(g_image_layer1, coord);
}


>
> FTR the reason why I didn't bother to introduce any format qualifier
> checks whatsoever in verify_image_parameter() is the following GLSL spec
> text from section 4.10 "Memory Qualifiers":
>
> | Layout qualifiers cannot be used on formal function parameters, but
> | they are not included in parameter matching.
>
> which sounds like it's legal to pass image variables of *any* format as
> parameter for any user-defined function -- Which brings me to the
> question of what should happen when you have e.g. an image atomic call
> inside a function passing an image formal parameter as argument to the
> atomic call -- Is that supposed to be an error?  If not how do you tell
> whether the format matches to emit the error above?  You don't?  If you
> don't how come it must be an error to do it at the top level?  Or is it
> not necessarily an error either and the CTS test is just checking
> out-of-spec behaviour?  The spec wording seems *really* sketchy
> honestly...
>

So it might be that CTS is out of spec, I'll see if I can dig up any
more info on it.
Dave.