[Mesa-dev] [PATCH 3/7] glsl: Forbid opaque variables as operands of the ternary operator.

[Francisco Jerez]

Ian Romanick <idr at freedesktop.org> writes:

> On 02/01/2015 01:15 PM, Francisco Jerez wrote:
>> Ian Romanick <idr at freedesktop.org> writes:
>> 
>>> On 01/31/2015 09:54 PM, Francisco Jerez wrote:
>>>> ---
>>>>  src/glsl/ast_to_hir.cpp | 12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>>>> index 1ba29f7..783384e 100644
>>>> --- a/src/glsl/ast_to_hir.cpp
>>>> +++ b/src/glsl/ast_to_hir.cpp
>>>> @@ -1596,6 +1596,18 @@ ast_expression::do_hir(exec_list *instructions,
>>>>           error_emitted = true;
>>>>        }
>>>>  
>>>> +      /* From section 4.1.7 of the GLSL 4.50 spec (Opaque Types):
>>>> +       *
>>>> +       *  "Except for array indexing, structure member selection, and
>>>> +       *   parentheses, opaque variables are not allowed to be operands in
>>>> +       *   expressions; such use results in a compile-time error."
>>>> +       */
>>>> +      if (type->contains_opaque()) {
>>>> +         _mesa_glsl_error(&loc, state, "opaque variables cannot be operands "
>>>> +                          "of the ?: operator");
>>>> +         error_emitted = true;
>>>> +      }
>>>> +
>>>
>>> This is what the spec says, but I'm suspicious that other vendors may
>>> also allow it.  Can we poke at AMD and NVIDIA close-source drivers to
>>> see if they allow things like
>>>
>>>     x = texture(b ? sampler1 : sampler2, t);
>>>
>>> If they allow it, I'll submit a spec bug.  If they do not allow it, we
>>> shouldn't either.
>>>
>> Yes, they do [1].  But I don't think it would be a good idea to change
>> the spec.  For images it would be fine actually (as long as the
>> condition is dynamically uniform) because they're implemented as a
>> simple 32-bit integer index.  But for samplers the hell would break
>> loose because most back-ends expect to be able to determine the sampler
>> binding statically (up to a dynamically uniform array index if they do
>> gpu_shader5), so the fact is that it's already broken and will lead to a
>> crash even if we pretend to allow it.
>
> Well...
>
>     x = texture(b ? sampler1 : sampler2, t);
>
> is really the same as
>
>     x = (b) ? texture(sampler1, t) : texture(sampler2, t);
>
> so it seems reasonable for a compiler to "just make it work."

What in combination with function calls can give rise to a combinatorial
explosion potentially making the compiler emit code to sample from every
sampler in the program.  I'd rather change the representation of
samplers to be an actual index that can be passed around as a value,
assuming the hardware supports it.  That would also get us closer to
ARB_bindless_texture.

> The problem with having everyone follow the spec is there's probably
> some application that expects the non-conformant behavior.  We get
> enough of those bug reports. :)
>
I haven't seen any for this particular kind of non-conformant behavior,
maybe we should wait until one comes up before we deliberately make our
life more difficult.  In the meantime an application that needs this
additional flexibility to choose among a (potentially very large) set of
textures at run time can use ARB_bindless_texture, IMHO there is a good
reason for unextended GL not to support this.

> Although, if we currently crash on this sort of input, we're already
> worse than generating an unexpected error for the application.
>
>> [1] http://people.freedesktop.org/~currojerez/piglit-summary-image_load_store-kepler/results-all/spec/ARB_shader_image_load_store/compiler/expression-selection.frag.html
>> 
>>>>        ir_constant *cond_val = op[0]->constant_expression_value();
>>>>        ir_constant *then_val = op[1]->constant_expression_value();
>>>>        ir_constant *else_val = op[2]->constant_expression_value();
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150201/e9d0f197/attachment.sig>