[Mesa-dev] [PATCH v2 1/3] glsl: add glsl_external_mode() helper

[Timothy Arceri]

On 17/05/18 15:23, Timothy Arceri wrote:
> On 17/05/18 14:59, Marek Olšák wrote:
>> On Thu, May 17, 2018 at 12:50 AM, Timothy Arceri 
>> <tarceri at itsqueeze.com <mailto:tarceri at itsqueeze.com>> wrote:
>>
>>
>>
>>     On 17/05/18 14:36, Marek Olšák wrote:
>>
>>         On Wed, May 16, 2018 at 11:01 PM, Timothy Arceri
>>         <tarceri at itsqueeze.com <mailto:tarceri at itsqueeze.com>
>>         <mailto:tarceri at itsqueeze.com <mailto:tarceri at itsqueeze.com>>>
>>         wrote:
>>
>>
>>
>>              On 17/05/18 10:59, Marek Olšák wrote:
>>
>>                  On Sat, May 12, 2018 at 12:49 AM, Timothy Arceri
>>                  <tarceri at itsqueeze.com <mailto:tarceri at itsqueeze.com>
>>         <mailto:tarceri at itsqueeze.com <mailto:tarceri at itsqueeze.com>>
>>                  <mailto:tarceri at itsqueeze.com
>>         <mailto:tarceri at itsqueeze.com> <mailto:tarceri at itsqueeze.com
>>         <mailto:tarceri at itsqueeze.com>>>>
>>
>>                  wrote:
>>
>>                       This allows us to get the variable mode as
>>         specified by the
>>                  GLSL
>>                       spec. We will use this in the following patch.
>>                       ---
>>                         src/compiler/glsl/ast_to_hir.cpp | 73
>>                  ++++++++++++++++++++++++++++++++
>>                         1 file changed, 73 insertions(+)
>>
>>                       diff --git a/src/compiler/glsl/ast_to_hir.cpp
>>                       b/src/compiler/glsl/ast_to_hir.cpp
>>                       index 3bf581571e2..a7a9ac80769 100644
>>                       --- a/src/compiler/glsl/ast_to_hir.cpp
>>                       +++ b/src/compiler/glsl/ast_to_hir.cpp
>>                       @@ -4175,6 +4175,79 @@
>>                  apply_type_qualifier_to_variable(const struct
>>                       ast_type_qualifier *qual,
>>                            apply_image_qualifier_to_variable(qual, var,
>>         state, loc);
>>                         }
>>
>>                       +/**
>>                       + * Get the GLSL mode specified in for system
>>         values (if any).
>>                       + */
>>                       +static enum ir_variable_mode
>>                       +glsl_external_mode(enum ir_variable_mode
>>         internal_mode,
>>                       gl_shader_stage stage,
>>                       +                   unsigned slot) {
>>                       +   if (internal_mode == ir_var_system_value) {
>>                       +      switch (slot) {
>>                       +      case SYSTEM_VALUE_SUBGROUP_SIZE:
>>                       +         return ir_var_uniform;
>>                       +      case SYSTEM_VALUE_PRIMITIVE_ID:
>>                       +         if (stage == MESA_SHADER_GEOMETRY) {
>>                       +            return ir_var_shader_out;
>>
>>
>>                  System values are always inputs. This one is
>>         gl_PrimitiveIDIn.
>>
>>                       +         } else {
>>                       +            return ir_var_shader_in;
>>                       +         }
>>                       +      case SYSTEM_VALUE_TESS_LEVEL_OUTER:
>>                       +      case SYSTEM_VALUE_TESS_LEVEL_INNER:
>>                       +         if (stage == MESA_SHADER_TESS_CTRL) {
>>                       +            return ir_var_shader_out;
>>
>>
>>                  System values are always inputs, i.e. TESS_CTRL isn't
>>         allowed here.
>>
>>
>>              Hmm yeah your right I seem to have confused myself here a 
>> bit.
>>              However SYSTEM_VALUE_SUBGROUP_SIZE seems to be the odd one
>>         out in
>>              that its a uniform.
>>
>>
>>         Why is it uniform?
>>
>>
>>      From ARB_shader_ballot:
>>
>>
>>          (Add to the list of built-in uniform variable declaration)
>>
>>              uniform uint  gl_SubGroupSizeARB;
>>
>>
>> I think it means that gl_SubGroupSizeARB is a compile-time constant. 
>> It probably shouldn't be a system value.
>>
>> Marek
> 
> In that case my original series should be sufficient once this is changed:
> 
> https://patchwork.freedesktop.org/series/43049/

Actually making it a non system value wont remove the need for this 
patch (or similar). Since we would need to make sure its not redefined 
as anything other than a uniform anyway.