On 24 May 2016 at 06:56, Ilia Mirkin <imir...@alum.mit.edu> wrote: > On Thu, May 19, 2016 at 11:47 PM, Dave Airlie <airl...@gmail.com> wrote: >> From: Dave Airlie <airl...@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 <airl...@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. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev