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

[Ilia Mirkin]

On Mon, May 23, 2016 at 5:12 PM, Dave Airlie <airlied at gmail.com> wrote:
> 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).

Oh right, that makes sense. So if you've already defined the new var,
just remove the new thing.

+ (*new_var)->data.max_array_access = ir->data.max_array_access / 4;

That's wrong in both sides of the if though, no? Shouldn't it just be
new_size - 1 for the non-primitive-input case? And I don't even know
what it should be in the primitive input case since the outer array
should stay the same while the inner array should have new_size - 1.

>
>>> +      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.

How about asserting then?

>
>> 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.

Hm ok. I'd still prefer avoiding strcmp, but I'll leave it to you.