[Mesa-dev] [PATCH 2/4] glsl: rewrite clip/cull distance lowering pass
Ilia Mirkin
imirkin at alum.mit.edu
Mon May 23 20:56:51 UTC 2016
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...
> {
> }
>
> @@ -100,12 +122,15 @@ public:
> * Type of shader we are compiling (e.g. MESA_SHADER_VERTEX)
> */
> const gl_shader_stage shader_stage;
> + const char *in_name;
> + int total_size;
> + int offset;
> };
>
> } /* anonymous namespace */
>
> /**
> - * Replace any declaration of gl_ClipDistance as an array of floats with a
> + * Replace any declaration of 'in_name' as an array of floats with a
> * declaration of gl_ClipDistanceMESA as an array of vec4's.
> */
> ir_visitor_status
> @@ -114,7 +139,7 @@ lower_distance_visitor::visit(ir_variable *ir)
> ir_variable **old_var;
> ir_variable **new_var;
>
> - if (!ir->name || strcmp(ir->name, "gl_ClipDistance") != 0)
> + if (!ir->name || strcmp(ir->name, in_name) != 0)
> return visit_continue;
> assert (ir->type->is_array());
>
> @@ -134,56 +159,53 @@ lower_distance_visitor::visit(ir_variable *ir)
>
> this->progress = true;
>
> - 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?
> + unsigned new_size = (total_size + 3) / 4;
>
> /* Clone the old var so that we inherit all of its properties */
> *new_var = ir->clone(ralloc_parent(ir), NULL);
> -
> - /* And change the properties that we need to change */
> - (*new_var)->name = ralloc_strdup(*new_var, "gl_ClipDistanceMESA");
> - (*new_var)->type = glsl_type::get_array_instance(glsl_type::vec4_type,
> - new_size);
> - (*new_var)->data.max_array_access = ir->data.max_array_access / 4;
> -
> + (*new_var)->name = ralloc_strdup(*new_var, GLSL_CLIP_VAR_NAME);
> +
> + 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)));
> +
> + assert (ir->type->fields.array == glsl_type::float_type);
> +
> + /* And change the properties that we need to change */
> + (*new_var)->type = glsl_type::get_array_instance(glsl_type::vec4_type,
> + new_size);
> + (*new_var)->data.max_array_access = ir->data.max_array_access / 4;
> + } else {
> + /* 2D gl_ClipDistance (used for tessellation control, tessellation
> + * evaluation and geometry input, and tessellation control output).
> + */
> + assert((ir->data.mode == ir_var_shader_in &&
> + (this->shader_stage == MESA_SHADER_GEOMETRY ||
> + this->shader_stage == MESA_SHADER_TESS_EVAL)) ||
> + this->shader_stage == MESA_SHADER_TESS_CTRL);
> +
> + assert (ir->type->fields.array->fields.array == glsl_type::float_type);
> +
> + /* And change the properties that we need to change */
> + (*new_var)->type = glsl_type::get_array_instance(
> + glsl_type::get_array_instance(glsl_type::vec4_type,
> + new_size),
> + ir->type->array_size());
> + (*new_var)->data.max_array_access = ir->data.max_array_access / 4;
> + }
> ir->replace_with(*new_var);
> } else {
> - /* 2D gl_ClipDistance (used for tessellation control, tessellation
> - * evaluation and geometry input, and tessellation control output).
> - */
> - assert((ir->data.mode == ir_var_shader_in &&
> - (this->shader_stage == MESA_SHADER_GEOMETRY ||
> - this->shader_stage == MESA_SHADER_TESS_EVAL)) ||
> - this->shader_stage == MESA_SHADER_TESS_CTRL);
> -
> - *old_var = ir;
> - assert (ir->type->fields.array->fields.array == glsl_type::float_type);
> - unsigned new_size = (ir->type->fields.array->array_size() + 3) / 4;
> -
> - /* Clone the old var so that we inherit all of its properties */
> - *new_var = ir->clone(ralloc_parent(ir), NULL);
> -
> - /* And change the properties that we need to change */
> - (*new_var)->name = ralloc_strdup(*new_var, "gl_ClipDistanceMESA");
> - (*new_var)->type = glsl_type::get_array_instance(
> - glsl_type::get_array_instance(glsl_type::vec4_type,
> - new_size),
> - ir->type->array_size());
> - (*new_var)->data.max_array_access = ir->data.max_array_access / 4;
> -
> - ir->replace_with(*new_var);
> + ir->remove();
> }
>
> return visit_continue;
> @@ -219,7 +241,7 @@ lower_distance_visitor::create_indices(ir_rvalue *old_index,
> * creating expressions to calculate the lowered indices. Just create
> * constants.
> */
> - int const_val = old_index_constant->get_int_component(0);
> + int const_val = old_index_constant->get_int_component(0) + offset;
> array_index = new(ctx) ir_constant(const_val / 4);
> swizzle_index = new(ctx) ir_constant(const_val % 4);
> } else {
> @@ -236,14 +258,16 @@ lower_distance_visitor::create_indices(ir_rvalue *old_index,
> * shift because that's likely to be more efficient.
> */
> array_index = new(ctx) ir_expression(
> - ir_binop_rshift, new(ctx) ir_dereference_variable(old_index_var),
> + ir_binop_rshift,
> + new(ctx) ir_expression(ir_binop_add, new(ctx) ir_dereference_variable(old_index_var), new(ctx) ir_constant(offset)),
> new(ctx) ir_constant(2));
>
> /* Create the expression distance_index % 4. Do this as a bitwise
> * AND because that's likely to be more efficient.
> */
> swizzle_index = new(ctx) ir_expression(
> - ir_binop_bit_and, new(ctx) ir_dereference_variable(old_index_var),
> + ir_binop_bit_and,
> + new(ctx) ir_expression(ir_binop_add, new(ctx) ir_dereference_variable(old_index_var), new(ctx) ir_constant(offset)),
> new(ctx) ir_constant(3));
> }
> }
> @@ -557,18 +581,98 @@ lower_distance_visitor::visit_leave(ir_call *ir)
> return rvalue_visit(ir);
> }
>
> +namespace {
> +class lower_distance_visitor_counter : public ir_rvalue_visitor {
> +public:
> + explicit lower_distance_visitor_counter(void)
> + : in_clip_size(0), in_cull_size(0),
> + out_clip_size(0), out_cull_size(0)
> + {
> + }
> +
> + virtual ir_visitor_status visit(ir_variable *);
> + virtual void handle_rvalue(ir_rvalue **rvalue);
> +
> + int in_clip_size;
> + int in_cull_size;
> + int out_clip_size;
> + int out_cull_size;
> +};
> +
> +}
> +/**
> + * Count gl_ClipDistance and gl_CullDistance sizes.
> + */
> +ir_visitor_status
> +lower_distance_visitor_counter::visit(ir_variable *ir)
> +{
> + int *clip_size, *cull_size;
> +
> + if (!ir->name)
> + return visit_continue;
> +
> + if (ir->data.mode == ir_var_shader_out) {
> + clip_size = &out_clip_size;
> + 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...
> + return visit_continue;
> +
> + if (*clip_size == 0) {
> + if (!strcmp(ir->name, "gl_ClipDistance")) {
> + if (!ir->type->fields.array->is_array())
> + *clip_size = ir->type->array_size();
> + else
> + *clip_size = ir->type->fields.array->array_size();
> + }
> + }
> +
> + if (*cull_size == 0) {
> + if (!strcmp(ir->name, "gl_CullDistance")) {
> + if (!ir->type->fields.array->is_array())
> + *cull_size = ir->type->array_size();
> + else
> + *cull_size = ir->type->fields.array->array_size();
> + }
> + }
> + return visit_continue;
> +}
> +
> +void
> +lower_distance_visitor_counter::handle_rvalue(ir_rvalue **rv)
> +{
> + return;
> +}
>
> bool
> -lower_clip_distance(gl_shader *shader)
> +lower_clip_cull_distance(struct gl_shader_program *prog, gl_shader *shader)
> {
> - lower_distance_visitor v(shader->Stage);
> + int clip_size, cull_size;
> +
> + lower_distance_visitor_counter count;
> + visit_list_elements(&count, shader->ir);
> +
> + clip_size = MAX2(count.in_clip_size, count.out_clip_size);
> + cull_size = MAX2(count.in_cull_size, count.out_cull_size);
>
> + if (clip_size == 0 && cull_size == 0)
> + return false;
> +
> + lower_distance_visitor v(shader->Stage, "gl_ClipDistance", clip_size + cull_size, 0);
> visit_list_elements(&v, shader->ir);
>
> - if (v.new_distance_out_var)
> - shader->symbols->add_variable(v.new_distance_out_var);
> - if (v.new_distance_in_var)
> - shader->symbols->add_variable(v.new_distance_in_var);
> + lower_distance_visitor v2(shader->Stage, "gl_CullDistance", &v, clip_size);
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.
> + visit_list_elements(&v2, shader->ir);
> +
> + if (v2.new_distance_out_var)
> + shader->symbols->add_variable(v2.new_distance_out_var);
> + if (v2.new_distance_in_var)
> + shader->symbols->add_variable(v2.new_distance_in_var);
>
> - return v.progress;
> + return v2.progress;
> }
> --
> 2.5.5
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list