[Mesa-dev] [PATCH v2 20/31] glsl: add _mesa_glsl_parse_state object to is_lvalue()

[Nicolai Hähnle]

On 26.04.2017 06:25, Timothy Arceri wrote:
> On 24/04/17 20:35, Samuel Pitoiset wrote:
>> Yes, this is a bit hacky but we don't really have the choice.
>> Plain GLSL doesn't accept bindless samplers/images as l-values
>> while it's allowed when ARB_bindless_texture is enabled.
>
> Are you sure we need this? Can't we just set all the bindless qualifier
> flags to 0 when bindless is disable or something, and then just check if
> any of the flags were set on the var?
>
> Although I'm not sure the current change is so bad. Thoughts?

The problem with the alternative approach is that temporary 
ir_variable's are generated all over the place, and so you'd somehow 
need to decide in the constructor which bits to set. This seems very 
involved and fragile to me.

Better to acknowledge that yes, the spec simply changed its mind about 
whether samplers/images can be lvalues, and count our blessings that it 
doesn't really matter after ast_to_hir.

The places that end up passing NULL are places that are only sanity checks.

Patch is

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>


>
>>
>> The default NULL parameter is because we can't access the
>> _mesa_glsl_parse_state object in few places in the compiler.
>> One is_lvalue(NULL) call is for IR validation but other checks
>> happen elsewhere, should be safe.
>>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>> ---
>>   src/compiler/glsl/ast_function.cpp | 2 +-
>>   src/compiler/glsl/ast_to_hir.cpp   | 2 +-
>>   src/compiler/glsl/ir.cpp           | 4 +++-
>>   src/compiler/glsl/ir.h             | 8 ++++----
>>   4 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/compiler/glsl/ast_function.cpp
>> b/src/compiler/glsl/ast_function.cpp
>> index 1b90937ec8..00b930eb2c 100644
>> --- a/src/compiler/glsl/ast_function.cpp
>> +++ b/src/compiler/glsl/ast_function.cpp
>> @@ -283,7 +283,7 @@ verify_parameter_modes(_mesa_glsl_parse_state *state,
>>                                mode, formal->name,
>>                                actual->variable_referenced()->name);
>>               return false;
>> -         } else if (!actual->is_lvalue()) {
>> +         } else if (!actual->is_lvalue(state)) {
>>               _mesa_glsl_error(&loc, state,
>>                                "function parameter '%s %s' is not an
>> lvalue",
>>                                mode, formal->name);
>> diff --git a/src/compiler/glsl/ast_to_hir.cpp
>> b/src/compiler/glsl/ast_to_hir.cpp
>> index 943c25a224..f90ae3d09a 100644
>> --- a/src/compiler/glsl/ast_to_hir.cpp
>> +++ b/src/compiler/glsl/ast_to_hir.cpp
>> @@ -971,7 +971,7 @@ do_assignment(exec_list *instructions, struct
>> _mesa_glsl_parse_state *state,
>>             * The restriction on arrays is lifted in GLSL 1.20 and
>> GLSL ES 3.00.
>>             */
>>            error_emitted = true;
>> -      } else if (!lhs->is_lvalue()) {
>> +      } else if (!lhs->is_lvalue(state)) {
>>            _mesa_glsl_error(& lhs_loc, state, "non-lvalue in
>> assignment");
>>            error_emitted = true;
>>         }
>> diff --git a/src/compiler/glsl/ir.cpp b/src/compiler/glsl/ir.cpp
>> index 01f68a321c..b9c4452f83 100644
>> --- a/src/compiler/glsl/ir.cpp
>> +++ b/src/compiler/glsl/ir.cpp
>> @@ -24,6 +24,8 @@
>>   #include "main/core.h" /* for MAX2 */
>>   #include "ir.h"
>>   #include "compiler/glsl_types.h"
>> +#include "glsl_parser_extras.h"
>> +
>>     ir_rvalue::ir_rvalue(enum ir_node_type t)
>>      : ir_instruction(t)
>> @@ -1454,7 +1456,7 @@
>> ir_dereference_record::ir_dereference_record(ir_variable *var,
>>   }
>>     bool
>> -ir_dereference::is_lvalue() const
>> +ir_dereference::is_lvalue(const struct _mesa_glsl_parse_state *state)
>> const
>>   {
>>      ir_variable *var = this->variable_referenced();
>>   diff --git a/src/compiler/glsl/ir.h b/src/compiler/glsl/ir.h
>> index 584714ef82..bb900233ba 100644
>> --- a/src/compiler/glsl/ir.h
>> +++ b/src/compiler/glsl/ir.h
>> @@ -233,7 +233,7 @@ public:
>>        ir_rvalue *as_rvalue_to_saturate();
>>   -   virtual bool is_lvalue() const
>> +   virtual bool is_lvalue(const struct _mesa_glsl_parse_state *state
>> = NULL) const
>>      {
>>         return false;
>>      }
>> @@ -1934,9 +1934,9 @@ public:
>>      virtual bool equals(const ir_instruction *ir,
>>                          enum ir_node_type ignore = ir_type_unset) const;
>>   -   bool is_lvalue() const
>> +   bool is_lvalue(const struct _mesa_glsl_parse_state *state) const
>>      {
>> -      return val->is_lvalue() && !mask.has_duplicates;
>> +      return val->is_lvalue(state) && !mask.has_duplicates;
>>      }
>>        /**
>> @@ -1961,7 +1961,7 @@ class ir_dereference : public ir_rvalue {
>>   public:
>>      virtual ir_dereference *clone(void *mem_ctx, struct hash_table *)
>> const = 0;
>>   -   bool is_lvalue() const;
>> +   bool is_lvalue(const struct _mesa_glsl_parse_state *state) const;
>>        /**
>>       * Get the variable that is ultimately referenced by an r-value
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.