[Mesa-dev] [PATCH 2/4] glsl: rewrite clip/cull distance lowering pass

[Dave Airlie]

On 24 May 2016 at 06:56, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Thu, May 19, 2016 at 11:47 PM, Dave Airlie <airlied at gmail.com> wrote:
>> From: Dave Airlie <airlied at redhat.com>
>>
>> The last version of this broke clipping, and I had to spend
>> sometime getting this working properly.
>>
>> I had to introduce a third pass to count the clip/cull totals,
>> all due to one messy corner case. We have a piglit test
>> tes-input-gl_ClipDistance.shader_test
>> that doesn't actually output the clip distances, it just passes
>> them like a varying from TCS->TES, the older lowering pass worked
>> but to lower clip/cull we need to know the total number of clip+culls
>> used to defined the new variable correctly, and to offset culls
>> properly.
>>
>> This adds an extra pass that works out the sizes for clip/cull,
>> then lowers gl_ClipDistance then gl_CullDistance into the new
>> gl_ClipDistanceMESA.
>>
>> The pass checks using the fixed array sizes code if they array
>> has been referenced, or is actually never used, and ignores
>> it in the latter case.
>>
>> Signed-off-by: Dave Airlie <airlied at redhat.com>
>> ---
>>  src/compiler/glsl/ir_optimization.h  |   2 +-
>>  src/compiler/glsl/linker.cpp         |   2 +-
>>  src/compiler/glsl/lower_distance.cpp | 220 ++++++++++++++++++++++++++---------
>>  3 files changed, 164 insertions(+), 60 deletions(-)
>>
>> diff --git a/src/compiler/glsl/ir_optimization.h b/src/compiler/glsl/ir_optimization.h
>> index 5fc2740..71b10e4 100644
>> --- a/src/compiler/glsl/ir_optimization.h
>> +++ b/src/compiler/glsl/ir_optimization.h
>> @@ -119,7 +119,7 @@ bool lower_variable_index_to_cond_assign(gl_shader_stage stage,
>>      bool lower_temp, bool lower_uniform);
>>  bool lower_quadop_vector(exec_list *instructions, bool dont_lower_swz);
>>  bool lower_const_arrays_to_uniforms(exec_list *instructions);
>> -bool lower_clip_distance(gl_shader *shader);
>> +bool lower_clip_cull_distance(struct gl_shader_program *prog, gl_shader *shader);
>>  void lower_output_reads(unsigned stage, exec_list *instructions);
>>  bool lower_packing_builtins(exec_list *instructions, int op_mask);
>>  void lower_shared_reference(struct gl_shader *shader, unsigned *shared_size);
>> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
>> index b856631..4b5b32c 100644
>> --- a/src/compiler/glsl/linker.cpp
>> +++ b/src/compiler/glsl/linker.cpp
>> @@ -4639,7 +4639,7 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>>          goto done;
>>
>>        if (ctx->Const.ShaderCompilerOptions[i].LowerCombinedClipCullDistance) {
>> -         lower_clip_distance(prog->_LinkedShaders[i]);
>> +         lower_clip_cull_distance(prog, prog->_LinkedShaders[i]);
>>        }
>>
>>        if (ctx->Const.LowerTessLevel) {
>> diff --git a/src/compiler/glsl/lower_distance.cpp b/src/compiler/glsl/lower_distance.cpp
>> index 301afe4..f21a1be 100644
>> --- a/src/compiler/glsl/lower_distance.cpp
>> +++ b/src/compiler/glsl/lower_distance.cpp
>> @@ -45,19 +45,41 @@
>>   * LowerCombinedClipCullDistance flag in gl_shader_compiler_options to true.
>>   */
>>
>> +#include "main/macros.h"
>>  #include "glsl_symbol_table.h"
>>  #include "ir_rvalue_visitor.h"
>>  #include "ir.h"
>>  #include "program/prog_instruction.h" /* For WRITEMASK_* */
>>
>> +#define GLSL_CLIP_VAR_NAME "gl_ClipDistanceMESA"
>> +
>>  namespace {
>>
>>  class lower_distance_visitor : public ir_rvalue_visitor {
>>  public:
>> -   explicit lower_distance_visitor(gl_shader_stage shader_stage)
>> +   explicit lower_distance_visitor(gl_shader_stage shader_stage,
>> +                                  const char *in_name, int total_size,
>> +                                  int offset)
>>        : progress(false), old_distance_out_var(NULL),
>>          old_distance_in_var(NULL), new_distance_out_var(NULL),
>> -        new_distance_in_var(NULL), shader_stage(shader_stage)
>> +        new_distance_in_var(NULL), shader_stage(shader_stage),
>> +        in_name(in_name), total_size(total_size), offset(offset)
>> +   {
>> +   }
>> +
>> +   explicit lower_distance_visitor(gl_shader_stage shader_stage,
>> +                                  const char *in_name,
>> +                                  const lower_distance_visitor *orig,
>> +                                  int offset)
>> +      : progress(false),
>> +       old_distance_out_var(NULL),
>> +        old_distance_in_var(NULL),
>> +       new_distance_out_var(orig->new_distance_out_var),
>> +        new_distance_in_var(orig->new_distance_in_var),
>> +       shader_stage(shader_stage),
>> +        in_name(in_name),
>> +       total_size(orig->total_size),
>> +       offset(offset)
>
> Please don't mix tabs and spaces. Seems like this has a healthy mix
> with every other line alternating style...

remote emacs editing, doesn't always pick up the style files, will fix that.
>>
>> -   if (!ir->type->fields.array->is_array()) {
>> -      /* gl_ClipDistance (used for vertex, tessellation evaluation and
>> -       * geometry output, and fragment input).
>> -       */
>> -      assert((ir->data.mode == ir_var_shader_in &&
>> -              this->shader_stage == MESA_SHADER_FRAGMENT) ||
>> -             (ir->data.mode == ir_var_shader_out &&
>> -              (this->shader_stage == MESA_SHADER_VERTEX ||
>> -               this->shader_stage == MESA_SHADER_TESS_EVAL ||
>> -               this->shader_stage == MESA_SHADER_GEOMETRY)));
>> +   *old_var = ir;
>>
>> -      *old_var = ir;
>> -      assert (ir->type->fields.array == glsl_type::float_type);
>> -      unsigned new_size = (ir->type->array_size() + 3) / 4;
>> +   if (!(*new_var)) {
>
> This is a new check, the old code didn't have. When is *new_var going
> to be null?
>

Always at the start, the old code always assigned it, since it was all done
in a single pass, since we do two passes once for clip, once for cull,
but we only ever want to define one new_var (gl_ClipDistanceMESA).

>> +      cull_size = &out_cull_size;
>> +   } else if (ir->data.mode == ir_var_shader_in) {
>> +      clip_size = &in_clip_size;
>> +      cull_size = &in_cull_size;
>> +   } else
>> +      return visit_continue;
>> +
>> +   if (ir->type->is_unsized_array())
>
> || !ir->type->is_array() presumably? If some jerk did "float
> gl_CullDistance", the below would go rather poorly...

I don't think the compiler lets you do float gl_ClipDistance
or gl_CullDistance. It's definitely not something I need to care
about at this depth.

> Would it be cleaner to identify variables by location rather than
> name? e.g. what happens if I don't have cull enabled, and use that as
> a regular ol' varying name. Whereas the only way to have a
> VARYING_SLOT_CULL position is to go via the proper built-in path.

gl_ names aren't user useable, the compiler would block it.

Dave.